Skip to content

Issue 434: Fixed imprecise crefs in XML Docs #485

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 16, 2018
Merged

Issue 434: Fixed imprecise crefs in XML Docs #485

merged 1 commit into from
Jul 16, 2018

Conversation

markusweimer
Copy link
Member

This fixes a couple of dangling cref in the XML Docs. This commit doesn't contain functional changes to the code.

Issue:
This closes #434

@dnfclas
Copy link

dnfclas commented Jul 3, 2018 •
edited
Loading

CLA assistant check
All CLA requirements met.

@markusweimer
Copy link
Member Author

The test failure seems odd, given that there are no functional changes in this PR.

codemzs
codemzs reviewed Jul 3, 2018
View reviewed changes
test/Microsoft.ML.TestFramework/TestCommandBase.cs Outdated
@@ -15,6 +15,7 @@
using Microsoft.ML.Runtime.Tools;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.ML.Runtime.Command;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order

src/Microsoft.ML.Data/Depricated/Vector/VBufferMathUtils.cs
@@ -350,7 +350,7 @@ public static void AddMultWithOffset(ref VBuffer<Float> src, Float c, ref VBuffe
/// Perform in-place scaling of a vector into another vector as
/// <c><paramref name="dst"/> = <paramref name="src"/> * <paramref name="c"/></c>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we have this depricated folder. Can you please check if the functions in here have any references?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They certainly do. I have no idea why this was put into deprecated, maybe @Ivanidzo4ka knows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also spelled as "Depricated" :(

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Jul 3, 2018 •
edited by TomFinley
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an responsible adult I would totally push my fault on shoulders of others. It appears to have been this way in the migrated codebase for some years, for no particular reason.

In reply to: 199894576 [](ancestors = 199894576)

Copy link
Contributor

@TomFinley TomFinley Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds legit.

TomFinley
TomFinley approved these changes Jul 5, 2018
View reviewed changes
Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not object to the change per se, but I wonder if we can have a policy if whether Rider support is actually a goal of this project, especially when it appears that the problem is that they have a bug with their doc comment parser. (Or else, VS is too permissive.) It's not immediately obvious to me that it should be a goal.

sharwell
sharwell suggested changes Jul 5, 2018
View reviewed changes
Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build should be updated to validate these at the same time the change is made. I can assist with this but wanted to mark the PR as soon as I noticed.

@markusweimer
Copy link
Member Author

I do not object to the change per se, but I wonder if we can have a policy if whether Rider support is actually a goal of this project,

Good point. I'd say no to that. As long as our builds work from the command line on all the platforms we target, we can stay out of the business of recommending or "supporting" tools.

What tool do we use to render API docs? If that tool can properly link the cref instances mentioned here, I am ok with closing this as a WONTFIX.

especially when it appears that the problem is that they have a bug with their doc comment parser. (Or else, VS is too permissive.)

I don't know enough about the rules for XML Docs. From what I understand from the warnings I got, we have two kinds of fixes in this PR:

  • cref pointing to a class that wasn't imported: Not raising an issue in VS here seems like overly lenient. How does the reader know which class is being referenced? Are there heuristics to be applied?
  • cref missing type parameters: I can see cases here where the type parameters aren't necessary to uniquely identify the class. However, not adding them seems brittle in the face of future change, as ambiguity could be introduced at a later time. And without tooling to raise an alert then, our documentation is at risk of deteriorating.

Hence, I currently believe that this PR is rooted in VS being more permissive than Rider / ReSharper.

sharwell
sharwell suggested changes Jul 5, 2018
View reviewed changes
Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further review, documentation comments are already validated during the build, and none of the locations changed by this pull request are ambiguous or problematic for the compiler. If a tool is failing to correctly read these references in source code, a bug should be filed for that tool because there is no validation we can automatically perform to ensure the comments stay "correct" with respect to those bugs in the future.

@sharwell
Copy link
Contributor

sharwell commented Jul 5, 2018 •
edited
Loading

@markusweimer helped narrow down the sources of behavior differences. I'm now neutral on this pull request (it's implemented correctly even if it's not required by the compiler), but believe that the specific changes in #499 are important to avoid problems as development progresses.

📝 Since I'm not a core reviewer on this repository, GitHub does not allow me to dismiss my previous "request changes". It's not a blocking review either way, but please consider it effectively dismissed.

@shauheen
Copy link
Contributor

@codemzs are your comments resolved?

@markusweimer
Copy link
Member Author

With #499 merged, I'd like to rebase this and address @codemzs's comments at the same time.

Issue 434: Fixed imprecise crefs in XML Docs
0c2223a 
This fixes a couple of dangling `cref` in the XML Docs. This commit
doesn't contain functional changes to the code.

Issue:
  This closes dotnet#434
@markusweimer
Copy link
Member Author

I have done a rebase and addressed @codemzs's comments.

@markusweimer
Copy link
Member Author

@codemzs, @TomFinley, @sharwell How shall this proceed? Can it be merged?

Ivanidzo4ka
Ivanidzo4ka approved these changes Jul 16, 2018
View reviewed changes
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

eerhardt
eerhardt approved these changes Jul 16, 2018
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@eerhardt eerhardt merged commit ef169b2 into dotnet:master Jul 16, 2018
eerhardt pushed a commit to eerhardt/machinelearning that referenced this pull request Jul 27, 2018
@eerhardt
Issue 434: Fixed imprecise crefs in XML Docs (dotnet#485)
d8fd3f7 
This fixes a couple of dangling `cref` in the XML Docs. This commit
doesn't contain functional changes to the code.

Issue:
  This closes dotnet#434
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@codemzs codemzs codemzs left review comments

@sharwell sharwell sharwell requested changes

@Ivanidzo4ka Ivanidzo4ka Ivanidzo4ka approved these changes

@eerhardt eerhardt eerhardt approved these changes

@TomFinley TomFinley TomFinley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documentation crefs point to nonexisting classes
8 participants
@markusweimer @dnfclas @sharwell @shauheen @codemzs @Ivanidzo4ka @eerhardt @TomFinley