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

Tidy up wizard code to be able to remove more StyleCop supression entries #868

Closed
mrlacey opened this issue Jul 21, 2017 · 11 comments
Closed
Assignees
Labels
Can Close Out Soon Work relating to this issue has been completed. meta Used to indicate the issue relates to how the project is run or the tool works. Wizard The issue relates to the wizard rather than the generated code.
Milestone

Comments

@mrlacey
Copy link
Collaborator

mrlacey commented Jul 21, 2017

The GlobalSupressions.cs file (in SolutionFiles folder and used by multiple projects) includes a large number of supressions that were only added to get the checks to pass. The intention was always that these would eventually be tied up. Now is the time to do that tidying.

Anything with the following justification should be removed and violations fixed or given a proper justification if we genuinely need the suppression.

"Initially suppressing everything before evaluating what we want and what's appropriate."

@mrlacey mrlacey added meta Used to indicate the issue relates to how the project is run or the tool works. Wizard The issue relates to the wizard rather than the generated code. labels Jul 21, 2017
@mrlacey mrlacey added this to the 1.3 milestone Jul 21, 2017
@crutkas
Copy link
Member

crutkas commented Jul 21, 2017

see issue #867, analyzer isn't in every project. Is that going to affect anything you have? I'll go through and start adjusting.

@crutkas
Copy link
Member

crutkas commented Jul 22, 2017

Once #867 inside #872 and #869 gets merged, we can roll onto this.

@mrlacey
Copy link
Collaborator Author

mrlacey commented Aug 15, 2017

All blocking issues (linked above) have been addressed.

@crutkas
Copy link
Member

crutkas commented Aug 15, 2017

woot!

mrlacey added a commit that referenced this issue Aug 24, 2017
audit was for #851
follow up task #868 already planned
@ralarcon ralarcon assigned dgomezc and unassigned ralarcon Aug 29, 2017
@ralarcon ralarcon added Microsoft on point Indicates that community contribution for this issue is not being sort. Typically because it relates and removed Microsoft on point Indicates that community contribution for this issue is not being sort. Typically because it relates labels Aug 29, 2017
@crutkas
Copy link
Member

crutkas commented Aug 30, 2017

is this realistic for 1.3? I would say lets focus on stability at this point.

@mrlacey
Copy link
Collaborator Author

mrlacey commented Aug 30, 2017

low priority, I vote to punt. This also potentially touches a lot of the wizard code (depending on how much gets addressed) and we should be heading more towards stabilization

@crutkas crutkas modified the milestones: 1.4, 1.3 Aug 30, 2017
@ralarcon ralarcon added the in-progress The issue is currently being actively worked on. label Oct 4, 2017
@dgomezc dgomezc added Can Close Out Soon Work relating to this issue has been completed. and removed in-progress The issue is currently being actively worked on. labels Oct 9, 2017
@sibille
Copy link
Collaborator

sibille commented Oct 11, 2017

Moving to 1.5, we removed a lot of supressions, but left some due to high impact

@sibille sibille removed the Can Close Out Soon Work relating to this issue has been completed. label Oct 11, 2017
@sibille sibille modified the milestones: 1.4, 1.5 Oct 11, 2017
@ralarcon
Copy link
Contributor

@mrlacey @crutkas Which is left mainly is the code order (private vars, public props, methods...) and I think this is not too much worthy compared to the complexity / amount of work needed. Can we close this issue?

@mrlacey mrlacey self-assigned this Oct 17, 2017
@mrlacey
Copy link
Collaborator Author

mrlacey commented Oct 18, 2017

I've added a PR which addresses a few more issues.
One of which was a one line change but was suppressed with the justification that it would have a "high impact"

I don't think "We follow the C# Core Coding Style and not require this rule." is a sufficient reason not to address SA1516 (separating elements with spaces)
It will be a big change but will improve readability and consistency to the code. Both these things improve maintainability of code and so save time in the long run. I'll happily make this change but wanted to call out this justification.
No, this isn't explicitly called out in the coding guidelines but that doesn't mean we shouldn't do it. This is also a really weak argument as some of the things I've fixed in the PR are explicitly required by the guidelines. If going to make justifications based on explicitly following the letter of the guidelines this needs to be done consistently to aid credibility and expectations.

I'd also like to address SA1401 (Fields must be private.)
Again this will be a big change but I'll happily do it--if noone else will. My concern with not doing this is that by allowing direct access to private variables we risk breaking encapsulation and that makes code less predictable and harder to understand. The complexity of the code is already high and we should do things to improve this and aid future support and enhancements.

@ralarcon
Copy link
Contributor

SA1516 We addressd others and probably copy an pasted the justification.
SA1401 I don't think there should be too much entries of this.

Agree to adress both

@mrlacey mrlacey added the Can Close Out Soon Work relating to this issue has been completed. label Oct 19, 2017
@sibille
Copy link
Collaborator

sibille commented Oct 20, 2017

Verified in dev

@sibille sibille closed this as completed Oct 20, 2017
@ghost ghost locked as resolved and limited conversation to collaborators Jun 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Can Close Out Soon Work relating to this issue has been completed. meta Used to indicate the issue relates to how the project is run or the tool works. Wizard The issue relates to the wizard rather than the generated code.
Projects
None yet
Development

No branches or pull requests

5 participants