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

Fixes #3572 and some others: Remove analyzers from solution #3600

Merged
merged 7 commits into from
Jul 11, 2024

Conversation

dodexahedron
Copy link
Collaborator

@dodexahedron dodexahedron commented Jul 10, 2024

This PR removes the enum source generator projects, code, pwsh module, and CI build item from v2.

The generated code from the v2 code at the base of this branch was copied into normal source files so that existing uses can be preserved.

Note that, if you change one of those enums, you need to add an entry to the FastIsDefined method's switch expression for that enum's extension class. Enum changes are considered breaking changes in the .net world, anyway, however.

Those files will get deleted at whatever point in the future I come back to the generators. Probably during beta.

Fixes

Proposed Changes/Todos

  • Remove analyzer projects
  • Remove analyzers from CI build
  • Remove Terminal.Gui.PowerShell.Analyzers PowerShell module
  • Preserve currently used generated code
  • Other miscellaneous cleanup mandated by the above

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
    - Not relevant
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

Ready to go

@dodexahedron dodexahedron requested a review from tig as a code owner July 10, 2024 21:37
@dodexahedron dodexahedron self-assigned this Jul 10, 2024
@dodexahedron dodexahedron added design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...) build-and-deploy Issues regarding to building and deploying Terminal.Gui dependencies Pull requests that update a dependency file testing Issues related to testing docs Can be resolved via a documentation update github_actions Pull requests that update GitHub Actions code v2 For discussions, issues, etc... relavant for v2 labels Jul 10, 2024
@dodexahedron dodexahedron added this to the V2 Alpha milestone Jul 10, 2024
@dodexahedron dodexahedron requested a review from BDisp July 10, 2024 22:34
Copy link
Collaborator

@BDisp BDisp left a comment

Choose a reason for hiding this comment

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

LGTM. I switch from develop and this PR branches and rebuild on both and it is working as expected without needing to run any scripts. Thanks.

@dodexahedron
Copy link
Collaborator Author

Good to hear.

I had tried in a couple of environments as well, but still wanted to be extra-sure I wasn't just getting lucky. 😅

I was originally splitting them off into a local nuget package but decided it wasn't worth wasting the effort on that since I'm already going to be doing something similar but not identical when I get back to it in the formal public nuget package I'll be putting out eventually.

Just wanted to work on other more important stuff for now and make your workflows easier in as simple a way as possible. There are other ways to alter your workflow that would have avoided the pain you guys were feeling, but it didn't feel worth altering those habits for something temporary.

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you. Thank you. Thank you.

@tig tig merged commit d3727e4 into gui-cs:v2_develop Jul 11, 2024
3 checks passed
@BDisp
Copy link
Collaborator

BDisp commented Jul 11, 2024

@dodexahedron It looks like you removed the unit tests. It would be beneficial to create them in the UnitTests project, but using TestHelpers to normalize the newlines regardless of whether the WSL tests are using the Windows 11 file system or not.

Comment on lines -62 to -66
<ProjectReference Include="..\Analyzers\Terminal.Gui.Analyzers.Internal\Terminal.Gui.Analyzers.Internal.csproj">
<PrivateAssets>all</PrivateAssets>
<OutputItemType>Analyzer</OutputItemType>
<ReferenceOutputAssembly>false</ReferenceOutputAssembly>
</ProjectReference>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You removed it here in #3600 but it's again on the v2_develop.

<ProjectReference Include="..\Analyzers\Terminal.Gui.Analyzers.Internal\Terminal.Gui.Analyzers.Internal.csproj">
<PrivateAssets>all</PrivateAssets>
<OutputItemType>Analyzer</OutputItemType>
<ReferenceOutputAssembly>false</ReferenceOutputAssembly>
</ProjectReference>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the analyzer project and its tests, yes.

Didn't remove the actual TG UnitTests project.

But good thought on reformatting and whatnot. I'll add what's necessary to make ReSharper ignore those files as part of #3558 too, so it doesn't change things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-and-deploy Issues regarding to building and deploying Terminal.Gui dependencies Pull requests that update a dependency file design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...) docs Can be resolved via a documentation update github_actions Pull requests that update GitHub Actions code testing Issues related to testing v2 For discussions, issues, etc... relavant for v2
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants