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

Clean code #19

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Clean code #19

merged 1 commit into from
Sep 19, 2023

Conversation

Ptosiek
Copy link
Contributor

@Ptosiek Ptosiek commented Sep 17, 2023

Hi,
I just passed a linter on the code and made a few parts more pythonic, I removed one or two things that were clearly not used/documented (like other GUI). I haven't touched any logic normally.

Remove other GUI options that are not implemented
Remove unused import + reduce scope of import exceptions
Proper check for None values (use is and not ==)
Use staticmethod + classmethod
Fix shadowing
Fix mutable default arg (There are actually more but they are fixed on other branches)
More pythonic way to check len
Simplify checks
Unused local symbols
Remove some obvious comments
Drop unused widget
Use os.path.join and os.makedirs
Fix some naming to be clearer

There's one breaking change because I renamed the folders to be "courses" and "screenshots". So they have to be manually renamed for existing data.

remove unused import + reduce scope of import exceptions
proper check for None values
staticmethod + classmethod
fix shadowing
fix mutable default arg
more pythonic way to check len
simplify checks
Unused local symbols
remove some obvious comments
drop unused widget
handle error on quit
use os.path.join
use os.makedirs
fix some naming to be clearer
@hishizuka hishizuka merged commit 1afb405 into hishizuka:master Sep 19, 2023
1 check passed
@hishizuka
Copy link
Owner

hishizuka commented Sep 19, 2023

Hi @Ptosiek, thanks for your PR.

I found one error, but your latest branch seems to have fixed it, so I'll wait for further PR.

modules/config.py 82L

  • (-) G_INSTALL_PATH = os.path.join(os.path.expanduser("~"), "pizero_bikecomputer")
  • (+) G_INSTALL_PATH = os.path.join(os.path.expanduser("~"), "pizero_bikecomputer/")
    (Need a slash "/" at the end of path)

qt.svg: Cannot open file '/home/pi/pizero_bikecomputerimg/back_white.svg', because: No such file or directory

Thanks to your fixes, I found three places that need to be fixed and one bug.
I will only modify it once I receive all your PRs.

fix-1
modules/config.py 913L
The check of G_HEATMAP_OVERLAY_MAP is not logical.

fix-2
api.py 551L 568L
You erased the argument cmd_print in the bluetooth_tethering method, but there are two places left in api.py.

fix-3
modules/sensor/ant_code.py is not needed, modules/sensor/ant/ant_code.py is needed.

fix-4(bug)
Heart Rate values for ANT+ are not being passed on properly.

@hishizuka
Copy link
Owner

Hi @Ptosiek ,

By the way, what code checking tool do you use?

@Ptosiek
Copy link
Contributor Author

Ptosiek commented Sep 20, 2023

Hi!
I used PyCharm Code inspector for this time. I would introduce flake8 later is possible.

  1. For the icon: You right for the icons, I had missed that icon_dir was set differently on the Pi.
    As you've seen I have removed that later. So OK if we can live with broken master for a bit. Though maybe it would make sense to have a dev branch and merge to master only working code in the future.

  2. G_HEATMAP_OVERLAY_MAP: thought so but again didn't want to change logic for now :)

  3. cmd_print in bluetooth. Indeed, I probably fixed on another branch that one.

I haven't removed files since I can't test all configurations. But it seems some could be dropped (pyqt_graph_debug.py at least)

I'll open a a few more PR (until I haven't made drastic changes)

@Ptosiek Ptosiek deleted the clean-code branch October 4, 2023 20:09
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.

2 participants