Identify and document internal code used by providers #11266
Comments
|
@ErikEJ @roji @caleblloyd Would appreciate your thoughts on this. |
|
@ajcvickers would you like me to open issues for the points raised in #11132? |
|
@roji That would be great, especially if you could include the actual internal types that are being used so we know where to go make annotations in our code. |
|
Pomelo.MySql Annotations Framework Conventions RevEng Logging Utility Command |
|
Npgsql Annotations Framework Conventions Query Logging Utility |
|
SqlCe/SqlServer/SQLite is hard to figure out at present because they use the same namespace as EFCore hence no way to differentiate between EFCore type vs provider type. Blocked on #11132 |
|
Thanks for the legwork @smitpatel. Just to say that although this is important, it isn't incredibly urgent for me - in general I try to keep up with EF Core so Internal changes at your end are usually a minor annoyance more than anything else. |
|
@smitpatel Look in this branch, there I have renamed namespace: https://github.com/ErikEJ/EntityFramework.SqlServerCompact/tree/2-1-preview1-prep |
|
@ajcvickers Sounds like a good plan, but agree on the urgency with @roji |
|
Thanks for compiling the list, @smitpatel. I agree with the others on the low urgency, we also try to do minor releases quickly that track upstream. It would be really nice to get to a point where minor and patch releases don't require many changes, because it can be time consuming figuring out what has changed upstream. |
|
sqlserver Annotations Framework Conventions Query Logging Utility Migrations/Scaffolding Update |
|
Sqlite Annotations Framework Conventions Query Logging Utility Update |
|
SqlServerCe Annotations Framework Conventions Query Logging Scaffolding |
smitpatel
added
the
closed-fixed
|
@TechnikEmpire Not sure how your comment relates to this "clean-up" issue? |
|
@TechnikEmpire Reading your first and second comments, I understand your intention was to point out that the EF team (not the Pomelo project) has made what you believe are several mistakes, starting with using .Internal namespaces as a way to indicate that certain APIs should be considered internal rather than using the available internal keyword, and that you believe that this had the effect to encourage actions that we should have tried to discourage. There were certainly reasons to do things the way we did them but I hope we can still take your feedback and use it to discuss if there are more things we can do around this area in the future (cc @ajcvickers). On the other hand, although this issue is closed and the children issues (#11404, #11405, #11406, #11407, #11408, #11409) are in the backlog, we are still planning to mitigate breaks caused by changes in 2.1. This means in some cases we will revert changes, in other cases we will tweak changes to avoid breaking and in some other cases changes will be necessary on providers. We will continue working with provider writers on this. Finally, in order to be able to count with your valuable feedback in the future, I need to ask you to follow the code of conduct at https://opensource.microsoft.com/codeofconduct/, in particular be respectful on your interactions with everyone in this repo. All comments you submit count, even if you delete or edit them later. |
|
@TechnikEmpire you are welcome to stay and continue participating of this conversation in a civil manner or to leave if you so desire. By “all comments you submit count”, I was referring to the fact that in my email I found comments from you that I no longer see in this thread. Yes, the code of conduct applies, because everyone that subscribed to this thread will also receive them. |
|
FWIW as a provider writer I can say that there has been a very good side to exposing internal types as public (under an Internal namespace). It has allowed me to implement functionality even if it involved using internal types, rather than completely blocking on them until the EF team makes them truly public and releases a version (or use reflection). That's allowed a very flexible/fast workflow, as long as I was willing to accept some breakage on the way and be reactive. This seems to be a good fit especially for a new product like EF Core where things aren't (or weren't) yet completely nailed down. It's worth reminding that unenforced encapsulation is how it works in some other languages (e.g. python has no public/internal/private), it's definitely a legitimate debate. Regarding the comments (which I also received via email), @TechnikEmpire you seem to have really gone off the deep end here (FYI I'm not part of Microsoft). |
|
@roji I agree re the public/internal surface. This is the main reason we did it. That said, it would have been great if you had to use more specific means to opt-in using an internal type in specific blocks of your code without getting a warning or error. The way things have been, it is apparently too easy for it to happen accidentally, and for us to miss the fact that an API that we consider internal is required to implement a provider. Changing namespaces can help a bit, but perhaps a feature more deeply integrated in the language could help more. |
|
@divega the namespace change is definitely important as we've seen. Maybe simply an analyzer check that produces warnings about usage of EF Core internal code? This way you'd be well-informed and can suppress the warnings if that's what you want. This could be useful for both providers and applications (which can also replace services etc.) |
|
Yes, an analyzer would be great. |
|
Enhance API consistency check and add it to the spec tests? |
|
@ErikEJ I think the problem with doing it in the spec tests is that it's actually OK to use internal code (after all that's why it's public), the main goal is to make sure people are aware that they're doing it. So there needs to be a way to say on specific usages "OK, I'm aware, I want to do it anyway". I'm not sure how such suppression could happen on a per-site basis outside of an analyzer-generated warning. |
|
@roji Analyzer suppressions ftw! |
|
Consolidated list of remaining work. (Will update individual issues as well, but wanted to have this in one place for me.) Annotations Framework
Conventions
RevEng
Logging
Utility
Command
Query
Update
InMemory stuff
|
Following up on the discussion from #11132, we have decided to take the following action for 2.1:
The text was updated successfully, but these errors were encountered: