Skip to content

Cleanups up and fixes docs in View class.#2098

Merged
tig merged 3 commits intogui-cs:developfrom
pavkam:cleanup_1
Oct 20, 2022
Merged

Cleanups up and fixes docs in View class.#2098
tig merged 3 commits intogui-cs:developfrom
pavkam:cleanup_1

Conversation

@pavkam
Copy link
Copy Markdown
Contributor

@pavkam pavkam commented Oct 18, 2022

This PR does not introduce functional changes. It merely modernizes one class. If this PR is accepted more code can be modernized subsequently.

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

Copy link
Copy Markdown
Member

@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.

My OCD and desire to always be removing code loves all this.

My fear of regressions does not.

I think we need more eyes on this before we merge it. I reviewed it fairly carefully, but I'd like @Tzind and @BDisp to do the same.

@tznind
Copy link
Copy Markdown
Collaborator

tznind commented Oct 19, 2022

Was this done with a tool? e.g. ReSharper? That would help my confidence in there being no mistakes a lot.

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 19, 2022

Was this done with a tool? e.g. ReSharper? That would help my confidence in there being no mistakes a lot.

I think so.

Comment thread Terminal.Gui/Core/View.cs
@tznind
Copy link
Copy Markdown
Collaborator

tznind commented Oct 19, 2022

Ok I've gone through it and all the changes look good and correct. Takes quite a while to comb through though, would be a big task to validate it across whole codebase.

That said the new code us much more readable and I really like it.

@pavkam
Copy link
Copy Markdown
Contributor Author

pavkam commented Oct 19, 2022

Was this done with a tool? e.g. ReSharper? That would help my confidence in there being no mistakes a lot.

I use Rider on a Mac. The IDE does highlight the issues but I do manually validate whether a change is OK or not. I have not acted upon all its hints as that would be too intrusive.

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 19, 2022

I use Rider on a Mac. The IDE does highlight the issues but I do manually validate whether a change is OK or not. I have not acted upon all its hints as that would be too intrusive.

That a good point. Really Rider show many unnecessary suggestions. I'm glad you didn't accept all them. Good work.

@pavkam
Copy link
Copy Markdown
Contributor Author

pavkam commented Oct 19, 2022

@BDisp @tig @tznind -- if you think it makes sense to go around cleaning up more of the source code, I am glad to do so. Gives me an opportunity to figure out the code.

I was looking into creating a TUI toolkit before for my own needs but it makes more sense to modernize parts of this project and continue from there.

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 19, 2022

@BDisp @tig @tznind -- if you think it makes sense to go around cleaning up more of the source code, I am glad to do so. Gives me an opportunity to figure out the code.

I was looking into creating a TUI toolkit before for my own needs but it makes more sense to modernize parts of this project and continue from there.

For me I'm glad for your interesting on improvement the Terminal.Gui. You only must understand that you have to wait until all the changes are careful checked before @tig merge it. You are welcome.

@tig
Copy link
Copy Markdown
Member

tig commented Oct 19, 2022

For the record, I'm still a tiny bit nervous about accepting this PR, but I really appreciate @pavkam's efforts and welcome him as a contributor!

Thanks to both @BDisp @tznind for reviewing.

In the future, any 'cleanups' like this should double down on more unit tests to reduce the risk of regressions.

I'm going to re-review this asap, just to make myself even more comfy with the changes...

@tznind
Copy link
Copy Markdown
Collaborator

tznind commented Oct 19, 2022

I was looking into creating a TUI toolkit before for my own needs but it makes more sense to modernize parts of this project and continue from there.

Awesome :) welcome aboard

Copy link
Copy Markdown
Member

@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.

Thanks again @pavkam! Welcome to the project.

@tig tig merged commit a108590 into gui-cs:develop Oct 20, 2022
This was referenced Nov 4, 2022
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.

4 participants