Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve examples for GetColumnName() breaking change #4016

Closed
wants to merge 1 commit into from

Conversation

Applesauce314
Copy link

Wording for provided example does not make it seem like an example
"Use the following code to get the column name for a specific table:"
see (#4024)

Wording for provided example does not make it seem like and example "Use the following code to get the column name for a specific table:"
see [https://github.com/dotnet/efcore/issues/28983]
@dnfadmin
Copy link

dnfadmin commented Sep 6, 2022

CLA assistant check
All CLA requirements met.

@Applesauce314
Copy link
Author

with dotnet 7 removing the obsolete overload, see (dotnet/efcore#26740), having better examples here is probably important.

@roji
Copy link
Member

roji commented Sep 6, 2022

@Applesauce314 the breaking change seems quite sufficient as it is, providing a code sample for exactly what you're looking for:

image

Your proposed code sample shows your specific usage of the API (looping over all properties in all entity types), which isn't appropriate in a breaking change note.

@Applesauce314
Copy link
Author

Applesauce314 commented Sep 6, 2022

@Applesauce314 the breaking change seems quite sufficient as it is, providing a code sample for exactly what you're looking for:

image

Your proposed code sample shows your specific usage of the API (looping over all properties in all entity types), which isn't appropriate in a breaking change note.

I disagree that the breaking change mitigation as is provides sufficient guidance. It states use this line of code, a line of code that did not work for my use case, (though I was able to figure out how to perform the necessary steps to get the needed StoreObjectIdentifier), The linked issue (#4024) is an example of someone who was not able to figure out the same steps on their own.

I tweaked the original "example" that required you to know the table and schema name, both to more clearly state it is an example, and to give a little more clarity on what exactly it is you are doing.

The second example was to provide a example where EntityType was know, but the name of the table was not. (e.g. loop through all the properties on all the entities) which I though was probably a common use case. (As it was for me and the creator of the linked issue)

I actually expected discussion on this second example as the loop through everything code will certainly encounter problems if the feature that necessitated the change to this API was utilized. (you just replied before I was able to make my comment to that effect)

@roji
Copy link
Member

roji commented Sep 6, 2022

The linked issue (#4024) is an example of someone who was not able to figure out the same steps on their own.

I suspect the problem in that issue is that the person did not see the breaking change note at all, otherwise they would have linked to it.

In any case, we'll discuss this as a team and see what people think.

ajcvickers added a commit that referenced this pull request Sep 21, 2022
ajcvickers added a commit that referenced this pull request Sep 21, 2022
ajcvickers added a commit that referenced this pull request Sep 21, 2022
@ajcvickers
Copy link
Member

Superseded by #4050.

@ajcvickers ajcvickers closed this Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants