Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Dec 8, 2020

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Dec 8, 2020

Can we get dependabot to do this for new releases?

@danmoseley
Copy link
Member

danmoseley commented Dec 8, 2020

@ViktorHofer have we considered dependabot or do we prefer more control?

@stephentoub
Copy link
Member

stephentoub commented Dec 8, 2020

More control, e.g. we want to be explicit about what rules are enabled, ensure our config file is explicit and complete, etc. And we don't need to update these every day / every time there's a stylecop update. Also, keep in mind this is an external piece of code that's run on everyone's machines.

@ViktorHofer
Copy link
Member

From my point of view, I'm not against using dependabot as it would nicely cover cases that aren't handled by darc.

@xtqqczze
Copy link
Contributor Author

@danmosemsft Can we merge this?

@sharwell
Copy link
Contributor

sharwell commented Dec 18, 2020

Before merging, can someone check if this repository uses any of the following rules?

  • SA1500
  • SA1502
  • SA1508

If so, you'll be affected by DotNetAnalyzers/StyleCopAnalyzers#3272 if you use the short syntax for record types (omitting {}). The fix is merged but not yet published.

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 18, 2020

Before merging, can someone check if this repository uses any of the following rules?

Those rules are currently disabled in both our ruleset files

<Rule Id="SA1500" Action="None" /> <!-- Braces for multi-line statements should not share line -->
<Rule Id="SA1501" Action="None" /> <!-- Statement should not be on a single line -->
<Rule Id="SA1502" Action="None" /> <!-- Element should not be on a single line -->
<Rule Id="SA1503" Action="None" /> <!-- Braces should not be omitted -->
<Rule Id="SA1504" Action="None" /> <!-- All accessors should be single-line or multi-line -->
<Rule Id="SA1505" Action="None" /> <!-- An opening brace should not be followed by a blank line -->
<Rule Id="SA1506" Action="None" /> <!-- Element documentation headers should not be followed by blank line -->
<Rule Id="SA1507" Action="None" /> <!-- Code should not contain multiple blank lines in a row -->
<Rule Id="SA1508" Action="None" /> <!-- A closing brace should not be preceded by a blank line -->
and
<Rule Id="SA1500" Action="None" /> <!-- Braces for multi-line statements should not share line -->
<Rule Id="SA1501" Action="None" /> <!-- Statement should not be on a single line -->
<Rule Id="SA1502" Action="None" /> <!-- Element should not be on a single line -->
<Rule Id="SA1503" Action="None" /> <!-- Braces should not be omitted -->
<Rule Id="SA1504" Action="None" /> <!-- All accessors should be single-line or multi-line -->
<Rule Id="SA1505" Action="None" /> <!-- An opening brace should not be followed by a blank line -->
<Rule Id="SA1506" Action="None" /> <!-- Element documentation headers should not be followed by blank line -->
<Rule Id="SA1507" Action="None" /> <!-- Code should not contain multiple blank lines in a row -->
<Rule Id="SA1508" Action="None" /> <!-- A closing brace should not be preceded by a blank line -->
.

@danmoseley
Copy link
Member

I don't object to merging per se but why the update? Why would we not wait for the next stable release, unless we hit a blocking bug?

@sharwell
Copy link
Contributor

I don't object to merging per se but why the update?

There are no changes from 1.2.0-beta.304 to 1.2.0-beta.312 that seem particularly interesting for this repository. The items of note are support for records without a block body, on the condition that you aren't using the three specific rules I pointed out above.

Why would we not wait for the next stable release ...

You'll be waiting a long time. Honestly it's not a problem unless you actually are seeing a problem.

... unless we hit a blocking bug?

The main reasons to update are:

  1. Improved performance (this is why 1.2.0-beta.304 was so beneficial; already adopted)
  2. Improved support for newer language versions (some of the 1.2.0 releases were merged for this)

@danmoseley
Copy link
Member

Thanks for that info. Given there's probably no benefit to the repo, I'm going to close this. We can consider picking up a new one when we would benefit from improved perf, a bug fix, new feature etc.

@xtqqczze thanks for offering the PR though, since it helps ensure that the version wouldn't cause a regression in this repo. No harm in doing that from time to time, but it could be a draft (CI still runs)

@danmoseley danmoseley closed this Dec 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 17, 2021
@xtqqczze xtqqczze deleted the upgrade-stylecop3 branch March 23, 2021 21:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants