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

Build: Common Nimble Hints #1514

Closed
kevinmatthes opened this issue Nov 15, 2022 · 4 comments
Closed

Build: Common Nimble Hints #1514

kevinmatthes opened this issue Nov 15, 2022 · 4 comments

Comments

@kevinmatthes
Copy link
Contributor

When working on #1513, I had a look at the output of the CI. The step "unit test" of the job "build ubuntu-latest" (test.yaml) generates various hints on style issues Nimble finds.

Here is an overview about common categories:

  • [ConvFromXtoItselfNotNeeded]
  • [DuplicateModuleImport]
  • [Name]
  • [XCannotRaiseY]
  • [XDeclaredButNotUsed]

The unit test generates more than 8000 lines of output, so I picked the most frequently appearing categories, for the beginning. Some points, like inconsistent spacing, are rather simple to fix and were only rarely reported in the CI's output.

The naming convention hints mostly concern the colours, but also other symbols in the code, such as the options for compilers (ClangOptionsclangOptions, for example). Since most of the hints Nimble generates are related to the naming conventions, I would like to suggest to defer the adjustment to dedicated enhancements of the concerning components and major refactorings, respectively, for example #1483, as the reason for the choice of a symbol's name for sure is more important than the respective naming convention being fulfilled in mostly any case. However, an adjustment would be a meaningful change in the long term as the compliance to the common conventions of the language is a sane goal, in my opinion.

The obsolete conversions and redundant imports are rather easy to fix, I assume that especially the removal of the casts would lead to some performance benefits. Again, often the colour management was affected.

Regarding the declared but unused symbols as well as the information that some procedures cannot raise certain exceptions I am unsure which of those points might belong to a long-term feature introduction and / or maintenance. Here, it should be discussed which changes can be made without breaking pending Pull Requests. Mostly, b2Mask, b3Mask, and b4Mask were flagged declared but never used; parsecannot raise Defect.

I would like to discuss how these points should be prioritised and which issues should be worked on first.

@fox0430
Copy link
Owner

fox0430 commented Nov 16, 2022

@kevinmatthes

Thank you for the report.
I think it's good to start with easy tasks. For example, obsolete conversions and redundant imports.
And, It may be a good idea to remove the hints displayed by nimble build --verbose first. There don't seem to be many of these.
Of course, If you are interested in something, you can start with that.

@kevinmatthes
Copy link
Contributor Author

@fox0430

Thank you for your answer.

I will begin to remove the obsolete conversions and redundant imports. As soon as I finished these adjustments, I will submit a new Pull Request.

@kevinmatthes
Copy link
Contributor Author

Most of the duplicate imports are seem to be caused by the inclusion of the respective source files to be tested.

@kevinmatthes
Copy link
Contributor Author

Solved by #1604.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants