Skip to content

Use EditorConfig to apply file headers#452

Merged
Sergio0694 merged 2 commits intoCommunityToolkit:mainfrom
Nirmal4G:build/file-header-template
Sep 28, 2022
Merged

Use EditorConfig to apply file headers#452
Sergio0694 merged 2 commits intoCommunityToolkit:mainfrom
Nirmal4G:build/file-header-template

Conversation

@Nirmal4G
Copy link
Copy Markdown
Contributor

@Nirmal4G Nirmal4G commented Sep 22, 2022

Changes

  • Use file_header_template to specify our header text in EditorConfig so that the supported editors can use it.
  • Remove the build/Update-Headers.ps1 script since the IDE Analyzer Fixer (IDE0073) took over its functionality.
  • Remove the build/header.txt as the text is embedded in EditorConfig file.

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • Tested code with current supported SDKs
  • Header has been added to all new source files (Use Visual Studio IDE0073 Code Fix)
  • Contains NO breaking changes
  • Code follows all style conventions

Other information

Rebase or Squash merge the PR and set its message to title and description.

@Nirmal4G
Copy link
Copy Markdown
Contributor Author

One issue I found is that, with script, we can customize where and how we check and apply the header text but with Roslyn analyzer fixer, only source files (i.e. Compile items) are used. Other files which might contain headers are not scanned.

Also, it seems to only warn/error in IDE and not in console builds. Do we need to provide an updated script that provides the missing features in the analyzer fixer?

@Nirmal4G Nirmal4G force-pushed the build/file-header-template branch from da95664 to f520a84 Compare September 22, 2022 16:28
@Nirmal4G Nirmal4G marked this pull request as ready for review September 22, 2022 16:31
@Nirmal4G Nirmal4G requested a review from Sergio0694 September 22, 2022 16:31
@Sergio0694
Copy link
Copy Markdown
Member

"it seems to only warn/error in IDE and not in console builds"

This is not good, we need the behavior to be the same and builds to fail if the header is not correct everywhere.
If that's no longer the case I don't think we can merge this. 🥲

@Nirmal4G
Copy link
Copy Markdown
Contributor Author

Nirmal4G commented Sep 26, 2022

we need the behavior to be the same and builds to fail if the header is not correct everywhere.

It does error out for both missing header and mismatched header but when I tried to build via dotnet build it doesn't seem to emit the error. But the examples and the videos show errors in the console. Am I missing something here?

Also, do we want file headers to be added for other files (other than .cs) in the repository?

@Sergio0694
Copy link
Copy Markdown
Member

The current behavior was just to add the header to .cs files, yeah. It's fine to omit it in other files. The important thing is that the CI should fail like before if any .cs file has an invalid header, so we do need the dotnet build validation in some way.

- Use 'file_header_template' to specify our header text
  in EditorConfig so that the supported formatters
  can use it automatically when formatting files.
- Previous Commit added support for Analyzer Fixer based file header
  addition to source code files. One was missing and wasn't updated
  through the script since the script only checks starting newline
  or a single comment line which were not enough to fix the file.

- Fortunately, the Analyzer caught this as IDE0073 style error and
  was able fix with a simple click on the suggestion icon in VS IDE.

NOTE: Previous Commit was supposed to fail to demonstrate
      build failure when using `file_header_template`.
@Nirmal4G Nirmal4G force-pushed the build/file-header-template branch from f520a84 to a264dfb Compare September 28, 2022 20:42
@Nirmal4G
Copy link
Copy Markdown
Contributor Author

Nirmal4G commented Sep 28, 2022

🎊 The build successfully failed! 🙌 I never thought I would say this with much enthusiasm. 😅

There was a file {MVVM.SourceGenerators}\Diagnostics\SuppressionDescriptors.cs which did not get the header added to it since there was no new line or comment line at the beginning of the file, which is what the script checks for and updates the file with our header text. 🧐

Now the IDE0073 throws for CLI builds. There's a MSBuild property $(EnforceCodeStyleInBuild) that must be set in-order for the Style Analyzers to kick-in. 😎

Copy link
Copy Markdown
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

This is awesome, love it! Thanks! 😄

@Nirmal4G
Copy link
Copy Markdown
Contributor Author

@michael-hawker I can't view pipeline console and logs anymore. The page errors out with 401-Unauthorized. I can access other repo's pipeline's public links but not this. Can you please check this?

Attached Screenshot
image

@Sergio0694 Sergio0694 merged commit e67754b into CommunityToolkit:main Sep 28, 2022
@Nirmal4G Nirmal4G deleted the build/file-header-template branch September 28, 2022 22:11
@michael-hawker
Copy link
Copy Markdown
Member

@Nirmal4G I'm not aware of any changes here, does the link open in an inprivate tab? The Azure DevOps can be pretty odd sometimes about authentication even though they should all be public links afaik.

@Nirmal4G
Copy link
Copy Markdown
Contributor Author

@michael-hawker Yep, it seems to open in InPrivate. 🤔 Why does InPrivate works but not regular way? I do clear all browsing data on session end even on regular tabs. Hmm..., Weird Issue!

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.

3 participants