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

Using NameOf when throwing exceptions #151

Merged
merged 8 commits into from Dec 5, 2018

Conversation

Projects
None yet
7 participants
@raffaeler
Contributor

raffaeler commented Dec 4, 2018

Created a new pull request as requested here

This pull request is a refactoring on all the argument exceptions specifying the name of the argument as hardcoded strings. All of them have been replaced with nameof(argumentname).
Some argument name were wrong, now they are fixed.

raffaeler added some commits Dec 1, 2018

Fixed and replaced all the occurrences of hard coded string with 'nam…
…eof(argument)'. Some argument name were wrong, now they are fixed

@raffaeler raffaeler requested a review from dotnet/dotnet-winforms as a code owner Dec 4, 2018

}
SetAudio(new MemoryStream(audioBytes));
}
/// <include file='doc\DataObject.uex' path='docs/doc[@for="DataObject.SetAudio"]/*' />

This comment has been minimized.

@JuditRose

JuditRose Dec 4, 2018

Member

Hi @raffaeler,
Thanks for the fixes you made!
Can you please exclude from this PR the changes which are also included in PR#148 (https://github.com/dotnet/winforms/pull/148/files)?

This comment has been minimized.

@raffaeler

raffaeler Dec 5, 2018

Contributor

@JuditRose Just sent a new commit for this. Let me know

@Tanya-Solyanik

This comment has been minimized.

Member

Tanya-Solyanik commented Dec 5, 2018

@raffaeler - did you run a Roslyn analyzer? Is it publicly available? It's difficult to review change of this size, but might be easier to review the analyzer code.

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@Tanya-Solyanik I wrote in the past a similar analyzer but I decided not run it because there are grayed regions and comments that are not covered from the changes.
So I changed strategy, created a regex and verified the replacements one by one.
Among all the changes, I did not replace a bunch of those because, for example, the string was referring to "DividerWidth" but the argument was "DividerThickness".

@MarcoRossignoli

This comment has been minimized.

MarcoRossignoli commented Dec 5, 2018

So I changed strategy, created a regex and verified the replacements one by one.

@raffaeler you could share a gist

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@MarcoRossignoli a gist for the regex? I u ndersdtand they need to verify the results one by one in any case.
@Tanya-Solyanik Sorry if the PR is large, but creating multiple PRs does not make that easier.

@MarcoRossignoli

This comment has been minimized.

MarcoRossignoli commented Dec 5, 2018

I undersdtand they need to verify the results one by one in any case.

I agree but maybe with gist they can verify code and re-apply amends and run diffs to understand if there are issues. There are also amends out of scope of PR https://github.com/dotnet/winforms/pull/151/files?utf8=%E2%9C%93&diff=split#diff-d59c3102cf61d3be2dd4889d505f16d9R1054 maybe could be better submit on other PRs. But I come from coreX repos and I don't know if the rules here are the same, let us know @Tanya-Solyanik

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

To be honest, I have a strong gut feeling that goes into the direction of @Tanya-Solyanik's question: I'd rather prefer an Analyzer for the code, and for the comments the RegEx would still work.

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@KlausLoeffelmann given that the original source code contained wrong hardcoded strings, a manual revision would be needed in any case. In addition to that, there are cases (the ones I cited in my comment above) where the hardcoded string does not match because it refers to a width while the parameter is a thickness.
I know it is boring, but the list of replcaments is not that long.

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@MarcoRossignoli changes other than the replacements are probably on a single file from a previous PR that also went closed accidentally. This PR is intended only for nameof replacements and nothing more. The fact I had to re-fork and redo the push request are probably the souce of the additional changes (which should affect one or two files, no more).

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

@raffaeler Changes would result in a compiler error one way or the other, but still an Analyzer would do this semantically not syntactically.

Said that, an Analyzer could already check the correctness/compilability of the resulting CodeFix, and probably add e.g. ObsoleteAttribute at those spots (for temporary tagging) instead of the intended CodeFix, where compilation would fail if the fix was applied for wrongly hardcoded strings.

So, this would result in 3 PRs, which less Risk, since after each Analyzer pass the solution would still compile:

a) Things the Analyzer could safely do.
b) Spots where the Analyzer left ObsoleteAttribute, and which needed to be resolved manually.
c) Comments and such via RegEx. (And maybe even this work could be done by the Analyzer?).

The more I think of this, the more I tend to an Analyzer. But I am curious what others of our team think?

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@KlausLoeffelmann The syntax is validated anyway by compiling, in this case the analyzer is not that different.
I wrote and use a lot of Roslyn based code, but more the analyzer is complex, more difficult is validating the analyzer code, blindly trusting the results.
That said, I do not agree on the ObsoleteAttribute and will let the team take the decision they want.

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

@raffaeler : My point is that with a RegEx pass, you could not test if the resulting code for each occurance would still compile, and you would necessarily need a manual cleanup before you'd able to check in. 'Obsolete' came to mind the second I wrote my reply, but you're right, it's not at all necessary: The Analyzer should just not apply any code fix in those cases, since it can find those spots for manual fixing, anyway.

My point is: I'd like to have a compilable solution at any time, and a RegEx pass simply does not guarantee that. My 2 cents.

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@KlausLoeffelmann compilation is always guaranteed by CI checks which were enforced as the repo went public. In addition to that I always compile and run the tests before a PR, and the same can be done by the team before approving the PR when the CI is not in place.

The only thing that can be wrong is that the argument inside nameof does not match with the intent of the coder and refers to the wrong argument (that still exists since the code compiles).
In that case a manual review is needed, regardless using an analyzer, a regex or whatever else.

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

Again, the main argument I have is, that if you applied the Analyzer, it should detect the cases, where the resulting fix would NOT compile, and leave it alone. That is certainly possible, right? So, that resulted in an intermediate result, which would not needed to be touched manually, would have a lot of spots fixed already, should be working, and could be tested and could go in. In my opinion, that would minimize the risk, because we'd break the big change into somewhat smaller changes (manually after automatically) which were much easier to review.

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

I understand what you are saying, but it doesn't change anything. The compile check is done in both cases.
We can discuss about the reliability of the code written on the analyzer that can be wrong as well even if the result compiles but at the very end comparing the two listing is the only way.
Again, the list of changes is not that long.

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

How would I recognize with RegEx, what came from the RegEx pass and what would have been manually edited?
(Without very consciously comparing every line during the review, of course)

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

How would you recognize the cases where the replacement logic has exceptions with "if" inside the analyzer?
The replacement is something like that:
From: throw new ArgumentOutOfRangeException("startIndex");
To: throw new ArgumentOutOfRangeException(nameof(startIndex));
Given that "startIndex" exists (otherwise it won't compile) and there were no manual changes, but just cases where I avoided to make the replacements, there is no other way that looking at the comparison inside the PR and verify it manually.
P.S. I find the discussion pretty useless as the team is the final judge on the way they want to proceed or drop the PR.

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

Sure, I will sit with my fellow team members as soon as they come online (I am working on the WinForms team remotely from Germany, hence already awake :-) ), and discuss this.

Still, to make sure, I understood correctly:
What would - in your example - the original code line be?

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

I am sorry, you included the original line.

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@KlausLoeffelmann in this case, just reject the PR by yourself ...

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

Let me rephrase:

If the Analyzer already checked that this Code compiled
throw new ArgumentOutOfRangeException(nameof(startIndex));
how likely is it, that this line is also correct?

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

In any case, analyzer or regex, after the replacement you just know the 'startIndex' argument is a valid token because the project compiles.
Since both the analyzer and the regex do the replacement starting from the string "startIndex", the initial intent of the developer who wrote the code is always preserved.

In case of // startIndex is a cool argument comment, the replacement cannot be done using the analyzer unless you search separately for the comment nodes and you have no guarantees the string 'startIndex' in the comment really matches with the token in the source code.

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

Rafaelle, please, give me just a chance to ask this question:
Let's assume for a moment, we cannot compile AFTER the fix had been applied.

(I would not go on your nerves like this, if not 190 (!) files would be affected, and I am sorry for being so adamant. When it turns out I have been unjustified concerned all along, I promise to eat humble pie!!)

Could we make the Analyzer in a way, so it would be sure to apply the CodeFix only when it has semantically proofed that the argument to be nameof'ed has really been passed or that, if it is inside a property's setter, that the Argument equals the property's name? And if not, that it does not apply a code fix, so leaving the original line in place?

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@KlausLoeffelmann oh please, the assumption is just wrong. We are in 2018 and CI / gated checkin is a must and is currently in place on this repo.
So please, let's not do philosophy.
Sorry, if you can't/want't reject the PR, let's wait for someone else but it is becoming a huge waste of time.

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 5, 2018

I am sorry you see it this way.
It's not at all my intension to reject a PR or a good idea.
I am just asking questions to get an idea about the risk, and if I am concerned, to minimize it.

@AdamYoblick

This comment has been minimized.

Member

AdamYoblick commented Dec 5, 2018

Hi @raffaeler, we're going to discuss this internally. I'm not sure if the contribution guidelines mention running an analyzer for PR's.

If we decide this is something we want to happen for all PRs, perhaps we should be running it automatically as part of our CI build or a git trigger instead of having contributors run it.

Going open is new to our team so we will have lots of wrinkles to iron out. Please be patient while we figure out what we want to do. Thanks :)

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@AdamYoblick thank you for the comment.
I remember to have carefully read the guidelines when I first submitted the PR (the one that was accidentally closed) when the repo was still private. I don't remember any mention to analyzers.
Anyway I understand the problem and the decision is yours.

In the past I had to make similar changes in a large codebase and, even if I already wrote an analyzer, I had to change strategy for the reason I mentioned.
All the best.

@karelz

I carefully reviewed all changes of pattern "abc" -> nameof(abc).
I found about 9 instances where the name of the var was changed - it would have been slightly better to have those changes in separate PR IMO (for future, I am fine with it here).

I found couple of unrelated changes (not the same pattern), they should be in separate PR - see comments above.

@karelz

This comment has been minimized.

Member

karelz commented Dec 5, 2018

BTW I believe @dotMorten has some analyzer doing similar things (for future use in other repos, etc.).

@raffaeler @MarcoRossignoli a gist for the regex? I understand they need to verify the results one by one in any case.
I wrote and use a lot of Roslyn based code, but more the analyzer is complex, more difficult is validating the analyzer code, blindly trusting the results.

Agreed. The change should be ideally verified one by one. I would not trust a regex unless I created it myself 😄 ... and even then ...
It would be different story if it was review taking hours. I would change my view point in such case. This one took ~15 mins, so ok-ish (for me).

For reference: For larger changes generated by tools/regex/etc., it is reasonable to ask for the source code of the tool, then verify it and run the same tool locally to verify the results match with the PR. Each person has different threshold when this triggers as a "must have", which is fine -- we are all different individuals with different preferences after all.

@raffaeler Sorry if the PR is large, but creating multiple PRs does not make that easier.

Agreed.
In future, the only minor improvement might be to separate out the "real" changes where the name was incorrect into separate PR. In this case, it is just 9 cases and makes reviewer alert 😉, so just nice to have IMO.

@karelz

This comment has been minimized.

Member

karelz commented Dec 5, 2018

Thanks @raffaeler for the change! It will make the code base more resilient in future, less prone to silly mistakes.
Also, sorry for the trouble of moving it from private repo to public one (it was new set of problems even for me).

If you can remove the unrelated changes from my code review, I will quickly review it, then I can squash & merge.

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@karelz I restored the two original files:

  • DataObject.cs I also renamed the hardcoded strings to nameof() to be consistent with this PR
  • DataStreamFromComStream.cs After committing I could see formatting differences made by VS. For this reason made a second commit "unformatting" the code as the original.
@karelz

karelz approved these changes Dec 5, 2018

@karelz karelz merged commit cccb7e7 into dotnet:master Dec 5, 2018

1 check passed

license/cla All CLA requirements met.
Details
@karelz

This comment has been minimized.

Member

karelz commented Dec 5, 2018

Thank you! Reviewed and merged!

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

Thank you!! Harder then expected :-)

@karelz

This comment has been minimized.

Member

karelz commented Dec 5, 2018

Isn't harder than expected the definition of SW engineer's daily life? 😉
I am almost daily surprised that things/approaches I think are obvious are viewed entirely differently by others. SW engineers thrive on disagreements and discussions (incl. myself) 😄.

@raffaeler

This comment has been minimized.

Contributor

raffaeler commented Dec 5, 2018

@karelz LOL, I can't live a single day of my life writing code even if most of the time I am involved in architectural roles (which I love too).
Until the discussion is just because of different technical perspectives it is always good even if there is disagreement.
Then we know that different cultures have different reactions. In this case travelling is the only treatment :)

@raffaeler raffaeler deleted the raffaeler:NameOf-Exceptions branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment