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 #3170. Color improvements #3204

Merged
merged 118 commits into from
Jan 24, 2024

Conversation

dodexahedron
Copy link
Collaborator

@dodexahedron dodexahedron commented Jan 22, 2024

Fixes

Proposed Changes/Todos

  • Significant re-working of Color, especially around formatting and parsing
  • Implement additional tests to cover the rest of the new code plus refactoring old tests

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)
  • 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

BDisp and others added 30 commits January 18, 2024 15:35
All-whitespace values are also illegal, so may as well handle that here too
All parsing is now almost-0 allocation, and is significantly faster than before
This commit won't build. I'm just breaking out changes a little bit.
@dodexahedron
Copy link
Collaborator Author

With the commit I pushed a few seconds ago, this is now ready to go.

Coverage of tests is at 100%, now, with that being a fairly thorough 100%.

@dodexahedron dodexahedron marked this pull request as ready for review January 23, 2024 20:30
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.

Fantastic stuff. See my comments which are all just about formatting and API docs.

@@ -105,6 +105,221 @@
<s:String x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/WRAP_MULTIPLE_DECLARATION_STYLE/@EntryValue">WRAP_IF_LONG</s:String>
<s:String x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/WRAP_OBJECT_AND_COLLECTION_INITIALIZER_STYLE/@EntryValue">CHOP_ALWAYS</s:String>
<s:String x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/WRAP_TERNARY_EXPR_STYLE/@EntryValue">WRAP_IF_LONG</s:String>
<s:String x:Key="/Default/CodeStyle/CodeFormatting/XmlDocFormatter/NamesOfTagsAlwaysOnNewLine/@EntryValue">summary,remarks,example,returns,param,typeparam,value,para,br</s:String>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is all this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted in the previous comment, but just responding here to close the loop.

Rider and ReSharper both have a ton of power, going above and beyond the basics included in stock VS, and they work much more reliably when configured with their own settings files, rather than letting it try to glean it from the .editorconfig and such, which doesn't always work perfectly and can result in some style preferences not being as desired.

Those specific things are what their tags suggest: Wrapping behavior when auto-formatting certain constructs. They are/can be further configured for specific lengths and other conditions at which wrapping/chopping happens. The last element, for example, tells it to always ensure those specific elements in XmlDoc start on a new comment line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On Rider/ReSharper: They are generally friendly to open-source projects and may be willing to grant you a low or no cost license if you'd like to take advantage of those powerful tools.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of you putting those in this PR, I think I'd prefer to get this all sorted as part of #3207 (comment).

One issue I discovered when I fell in love with resharper a few weeks ago, was everyone needs to use it or we get merge conflicts galore. While we have a OSS license for resharper for this project, it seems onerous to make it a requirement that any random contributor needs to use it.

Can you help us navigate that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Glad to, on both counts (separating that from this PR and onboarding with resharper).

You likely are or were encountering issues resulting from configuration that just didn't match desired formatting, as I was, though likely with a somewhat different rule set, since my base rule set is somewhat modified from stock, and ReSharper applies settings in layers.

With the appropriate settings, the output can of course match what Visual Studio would produce, while also providing facilities on top of that, such as the layout engine, which is super-customizable, and tons of analyzers and refactorings.

I'll delete the dotsettings files in a commit on this branch, so they don't exist at merge time, and will branch from that point to put them back in.

I also will define the majority of settings in the solution level file, and only put relevant overrides or additions in the project-level files, if or when relevant (for example, some settings related to unit tests clearly aren't relevant to non-test projects). That way, it'll be nice and clean, hierarchical, and easy to tweak at any given level down the line.

Be aware those files are not intended to be human-readable, and some features will write things out that are pretty hard to look at. But every element has a corresponding item in the ReSharper options UI, and that UI is fully searchable, all the way down to the names/labels of and, where textual, also the values of individual settings at all levels, including for plugins to ReSharper (which are managed separately from VS plugins).

My only warning is that ReSharper is crack for dotnet developers. Once you get a taste, you get hooked and never want to go without. 😅 But for real, it's a powerful force multiplier when you use it, and it will also teach you things about the language and just in general delivers peace, joy, and goodwill to all mankind. Or something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There ya go. Removed both of the dotsettings files from this branch. I'll recreate them in another branch as described above.

@dodexahedron
Copy link
Collaborator Author

dodexahedron commented Jan 23, 2024

On a personal note, I am SO pleased with and grateful for the fact that you actually care about documentation and its quality, and even bother to actually scrutinize it. ❤️

Documentation, especially for an API project, is so critical, and far too often doesn't get nearly the attention or effort it deserves.

So THANK YOU!

And keep it coming whenever you see anything. "Make it bleed" as my high school English teacher would say. 😅

@dodexahedron
Copy link
Collaborator Author

I'll put in the fixes as soon as I've got a DotSettings config that results in proper adherence to style preferences. I'm doing that right now, before I begin any other work.

@dodexahedron
Copy link
Collaborator Author

Requested changes made and pushed.

Let me know if there's anything else!

@dodexahedron
Copy link
Collaborator Author

Oh also I noticed that the new ColorExtensions class was still under the new namespace. I reverted that, and that namespace no longer exists, now.

@dodexahedron
Copy link
Collaborator Author

There we go.

That last commit should be all places where those new method calls also got extra spaces at the call-sites, so that should be all clean everywhere now.

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.

Really, really, great work. thank you.

@tig tig merged commit 92a6012 into gui-cs:v2_develop Jan 24, 2024
1 check passed
@dodexahedron
Copy link
Collaborator Author

Glad to help :)

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