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

Integrate navigation expansion explanations by @smitpatel into source comments #19415

Merged
merged 1 commit into from
Jan 18, 2020

Conversation

roji
Copy link
Member

@roji roji commented Dec 27, 2019

After seeing @smitpatel's awesome explanation of navigation expansion, I had the crazy idea of... integrating the comments into XML doc comments. In some cases I corrected grammar but can change back or whatever.

It would be great if we started to have a bit more doc comments, especially on complicated areas - I think they could help contributors/provider writers immensely.

@roji roji requested a review from smitpatel December 27, 2019 17:31
@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 27, 2019

Hmmm... Docs for internal?? Could that not be misleading?

@ajcvickers
Copy link
Member

@roji The xml docs for query need to be written in general. I was intending to talk to @smitpatel about this in the new year. We violated our docs rules for 3.0 because we needed to spend time on the code. We now need to go back and write the docs.

(I'm talking about public docs here. Internal docs like this are a lower priority.)

@roji
Copy link
Member Author

roji commented Dec 27, 2019

Hmmm... Docs for internal?? Could that not be misleading?

Hopefully not :) Where relevant I even document private members - the target is other devs working on the product rather than users consuming APIs.

@ajcvicker @smitpatel no problem, as Smit already wrote this stuff it seemed relevant to bring it into the code (for me it's not always been easy to grok all the components of navigation expansion). Obviously user-facing docs are more important.

@smitpatel
Copy link
Member

I recommend not doing this. Such explanation does not belong in XML docs. If we want to preserve it, we should find another way.

@roji
Copy link
Member Author

roji commented Jan 7, 2020

Let's discuss in design tomorrow - when going into new parts of EF Core I've frequently wished for these explanations. I do believe the source code is the right place for it (100% discoverable, shows up in the IDE).

@vslee
Copy link

vslee commented Jan 8, 2020

It would be nice to have the comments in the source, to make them more easily discoverable. If they were in a webpage, then people might not know where to look. And even for ppl who did know, they'd have to bounce back and forth between the webpage and the code.

@roji
Copy link
Member Author

roji commented Jan 8, 2020

@smitpatel I think this is up to you to review.

@roji
Copy link
Member Author

roji commented Jan 18, 2020

@ajcvickers removed method comment and rebased.

@roji roji merged commit a0bf7f0 into master Jan 18, 2020
@roji roji deleted the NavigationExpansionDocs branch January 18, 2020 19:24
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.

5 participants