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

General cleanup of code #2273

Open
5 of 8 tasks
MattHag opened this issue Feb 12, 2024 · 21 comments
Open
5 of 8 tasks

General cleanup of code #2273

MattHag opened this issue Feb 12, 2024 · 21 comments

Comments

@MattHag
Copy link
Collaborator

MattHag commented Feb 12, 2024

Information

  • Solaar version: 1.11rc (latest master)

Is your feature request related to a problem? Please describe.

  • Replace NamedInt, NamedInts with built-in Enums, which supports type checking and auto-completion
  • Type hint functions, methods, parameters ...
  • Although, setup.py mentions compatibility for Python 3.7 and above, there are still some additional code pieces to support Python 2. A cleanup should remove these workarounds.
  • Upgrade the code style to favor double quotes " for strings over single quotes ' to prepare for usage of f-strings with dictionaries.
  • Use Python 3 features, such as well-readable f-strings introduced in Python 3.6.
  • Remove all assertion from non-test code (Can be tackled along rewrites) not so easy
  • Let functions raise a dedicated error, instead of return None - in the error case
  • Remove humongous number of code lines in an init py
@MattHag MattHag changed the title Remove code constructs to support legacy Python 2 and Python 3 versions Remove code constructs to support Python 2 and legacy Python 3 versions Feb 12, 2024
@pfps
Copy link
Collaborator

pfps commented Feb 12, 2024

Yeah, the question is identifying what should be removed.

@MattHag
Copy link
Collaborator Author

MattHag commented Feb 12, 2024

At least the code pieces commented with Python 2, which I already removed and pushed.

Maybe 'Upgrade code to Python 3 style' is a better title, in order to upgrade strings to f-strings with pyupgrade as well and upgrade remaining strings manually.

pip install pyupgrade
pyupgrade --py37-plus `find lib/. -name "*.py" -type f`
pyupgrade --py37-plus `find lib/. -name "*.py" -type f`

@MattHag MattHag changed the title Remove code constructs to support Python 2 and legacy Python 3 versions Upgrade code style to Python 3 Feb 12, 2024
@MattHag
Copy link
Collaborator Author

MattHag commented Feb 13, 2024

In addition to that, I also suggest a switch to double-quoted strings "" instead of '', and an upgrade to the insanely fast ruff, which replaces isort, yapf and flake8. This tool is also quite handy as very quick 'on save' auto formatter.

#2295

@pfps
Copy link
Collaborator

pfps commented Feb 13, 2024

These seem reasonable, but let's defer this until after 1.11.1 goes out, which should be in a week. Then I have a bunch of changes to improve the imports loops, except for the big one of calling the UI from logitech_receiver.

@MattHag
Copy link
Collaborator Author

MattHag commented Feb 21, 2024

I think the mentioned issues have already been implemented.

However, when adapting the title to clean up, it could target

  • Remove empty hashtag lines #2515
  • Remove any commented out code
  • Replace all import ... as _... statements and favor loading the module.
  • Update license notice at the beginning of longstanding files and update license file, if necessary. Keep in mind, that sometimes comments are put after that legal part, where they are poorly visible.

@pfps
Copy link
Collaborator

pfps commented Feb 22, 2024

The advice I have received is that the license notice should be at the top of each file. It's probably overkill but that is what I have been told.

@MattHag
Copy link
Collaborator Author

MattHag commented Feb 22, 2024

Yes, it is and it is not beneficial for development and leads to some irritation for me.

It is obvious, that there's no real plan for maintaining them, since most of the years date back to 2012-2013. This needs a plan and the best is to remove unnecessary duplication and ambiguous years from the individual files.

  • Which name should be there?
    • If it is Solaar developers, is that even something good or does that refer to anyone contributing something. Could this have a weakening effect on the legal side?
  • What does the year refer to? 2013 is certainly not the year of the latest changes, but also not the introduction of the module.

@pfps
Copy link
Collaborator

pfps commented Feb 22, 2024

The important part of this section is the license. The code is Solaar uses the GPL and this can't be changed and should be prominently mentioned in each file.

It is true that the copyright line has not been updated. I'm now putting new information in for the files that I am touching.

@MattHag
Copy link
Collaborator Author

MattHag commented Feb 24, 2024

Good, then a cleanup and unification would be to replace all existing ones with that timeless message, as 90% of them are dated.
And furthermore reformat the module level descriptions, which are sometimes appended in the same style as the copyright notice. They use a # instead of a Multiline comment „““, which would be easy to distinguish.

@MattHag MattHag changed the title Upgrade code style to Python 3 General cleanup of code Feb 24, 2024
@MattHag
Copy link
Collaborator Author

MattHag commented Feb 28, 2024

It is true that the copyright line has not been updated. I'm now putting new information in for the files that I am touching.

This needs a pre-commit hook then, otherwise it will never be up-to-date.

MattHag added a commit to MattHag/Solaar that referenced this issue Feb 28, 2024
@pfps
Copy link
Collaborator

pfps commented Feb 28, 2024

The copyright lines are the least important part of the license block, or so I understand. Having them be out of date is not ideal but doesn't invalidate the license.

@MattHag
Copy link
Collaborator Author

MattHag commented Feb 28, 2024

Sure, but there's certainly a nice handy tool to maintain it. Or, I would not even add a date at all anymore.

pfps pushed a commit that referenced this issue Feb 29, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
pfps pushed a commit that referenced this issue Mar 2, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
The basic files in root often use all capitals, as is already used for
readme and manifest.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Mar 2, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
The charge status is solely used in the hiddpp20 module, thus put it
into this module.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
Ensure behavior stays the same.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
Classes shouldn't don't need to know about other settings classes.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
Enforce create_widgets and collect_values.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
Refactor code related to task and task ID.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
The key flags are solely used in hiddpp20 module, thus put them into the
module.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
This data is not in use currently.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
The mapping flags are solely used in hiddpp20 module, thus put them into
this module.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
The charge status is solely used in the hiddpp20 module, thus put it
into this module.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
Ensure behavior stays the same.

Related pwr-Solaar#2273
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
MattHag added a commit to MattHag/Solaar that referenced this issue Dec 1, 2024
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

3 participants