Huge push to improve codefactor

[bosd]

The first of all, my apologies for this huge PR! There is a little story behind this.

This is the result after extensive cleaning and testing. It started out as an automatic cleanup. Running black, isort, prettier, flake8 and so on. This already was a huge push towards a better codefactor score.

However, some fixups were needed. As some code execution was changed by re-ordering the code. As some code which was previously dormant got activated due to difference in indentation. After the Fixups, testing was done on a linux machine on python 3.8 and a windows machine on python 3.10

In my opinion it is ready to be a new major release and merging.

Is all the Cleanup done?? Sadly, No, Flake8 still gives some messages about unused variables,imports and so on. But you can't have it all (at once) ;)

[bosd]

Oops, commit list is very long. I could squash it. But don't now if it would make things more clear on such a large commit. Let me know your thoughts..

[m1ch]

Some %-string formating has been changed, but not all. I probose to serch for all %s %f %d %c %g and replase by str.format() as well.

[m1ch]

I reviewed all python files and consider them fine. Thank you @bosd for your work. It was also my first thought to do the same, before start working at actual changes.

@vlachoudis @Harvie - please consider merging this PR soon, as it brings the entire code-base to a good level.

[bosd]

Some %-string formating has been changed, but not all. I probose to serch for all %s %f %d %c %g and replase by str.format() as well. Thanks for the suggestion. I was'nt fully aware of these replacements.

Dunno, if I would like to incorporate it into this pr. (already spent more time on it than I should) I'm all in to quickly get this one ready for merging.

There is one "issue" left with this code. On a windows(10) machine the window icons are not correctly set. In this PR I've added the new method (sorry, don't know the actual name) for setting the logo icon. But as the old code still is in place. It will overrride the logo assignment and the regular tkinter logo is shown in the titlebar.

My proposal: Depreciate the old (python2??) code for logo assignment. in a different PR.

[m1ch]

Some %-string formating has been changed, but not all. I probose to serch for all %s %f %d %c %g and replase by str.format() as well.

Thanks for the suggestion. I was'nt fully aware of these replacements.

Dunno, if I would like to incorporate it into this pr. (already spent more time on it than I should) I'm all in to quickly get this one ready for merging.

I did a quick check and found almost 900 %-strings, in 56 files, left in your PR, so I understand, that you don't want to do it. % - formatting is discouraged, but will not be deprecated any time soon.

There is one "issue" left with this code. On a windows(10) machine the window icons are not correctly set. In this PR I've added the new method (sorry, don't know the actual name) for setting the logo icon. But as the old code still is in place. It will overrride the logo assignment and the regular tkinter logo is shown in the titlebar.

My proposal: Depreciate the old (python2??) code for logo assignment. in a different PR.

I encourage dropping python2 support entirely (#1719) since it is EOL for quite some time now.

[bosd]

I encourage dropping python2 support entirely (#1719) since it is EOL for quite some time now.

Me too. Would like to know the stand of @vlachoudis and @Harvie on python2 support and this pr.

1. Create a release of the current repo (before this merge) as the last python 2 supported version.
2. Merge this PR as a new major release. In this current state? or with dropped python2 support and more cleanup? (We're gonna need some more help and contributors on this PR for a full code cleanup)

I did a quick check and found almost 900 %-strings, in 56 files, left in your PR, so I understand, that you don't want to do it. % - formatting is discouraged, but will not be deprecated any time soon.

There is some (semi) automated way to achieve this. But if I'm not mistaking this would mean that python 3.6 or newer is required. Not sure if we want to do this in this PR. As this one is already huge!

[Harvie]

I think dropping python 2 is OK.

[Harvie]

I know it's just affair of personal taste, but i am not a big fan of indentation using spaces. No matter what python reccomends. Any chance to reindent using tabs? :-)

[Harvie]

I don't really think that breaking lines in CHANGELOG.md adds to readability

[Harvie]

Appreciate the efforts, but this PR seems rather huge and i am not sure when i will be able to do a full review.

[Harvie]

Actualy i beleive this PR might get bit smaller if you use tabs for indentation, because that is what i've used previously. that might make it bit easier to review.

[Harvie]

What codefactor score did you managed to achieve? :-)

[m1ch]

I know it's just affair of personal taste, but i am not a big fan of indentation using spaces. No matter what python reccomends. Any chance to reindent using tabs? :-)

I think that tabs should be avoided in code at all costs. Spaces are the standard in Python and, unless you have a project where you are not cooperating at all, it is the best practice to follow the guideline, like PEP8.

Appreciate the efforts, but this PR seems rather huge and i am not sure when i will be able to do a full review.

I did already a check on all python files and found no problems. I used meld for the comparison and configured it to ignore all whitespace characters. Furthermore, I think not replacing tab's with spaces would not reduce the amount of files that need to be reviewed, as there are lots of other stile guide violations as well.

I also run the branch to see any obvious problems.

[bosd]

What codefactor score did you managed to achieve? :-)

2568 issues fixed. 88 issues found. However, if you run flake8 there are still a lot of message which could be taken care of. Like unused var's and stuff. If we touch on the topic of code optimalization, a lot more could be achieved, but that's another topic.

Hoping this cleaner version of code, will make it easier to attract dev's.

I know it's just affair of personal taste, but i am not a big fan of indentation using spaces. No matter what python reccomends. Any chance to reindent using tabs? :-)

Well, I think a huge part of this PR is fixing indentation and replacing tabs with spaces. Indeed pyhthon recommends spaces. The replacement of tabs with spaces has been done by using a script and re-using black, isort, pre-commit settings. (used the settings of the OCA community). Don't see any way of undoing it easily in this pr. Personally would recommend to stick to the python / PEP8 recommendations and leave that in this PR as is. (The same way, readme is changed, propably, due to the line is longer then 80 chars)

I think dropping python 2 is OK.

Ok, let's drop it.

Appreciate the efforts, but this PR seems rather huge and i am not sure when I will be able to do a full review.

Understandibly a full review of code is a huge task. Maybe some functional testing? If ok, merge it.. If some new bug's are introduced we can fix it. (people can revert to the previous release)

[m1ch]

IMHO I think, that merging this PR next would be, that bosd:Cleancodev2 is still ahead of vlachoudis:master and no manual merge is necessary. As soon as there are other PRs merged in master, it will be a lot of work to bring the code up to date.

On the other hand, I was considering to fork bosd:Cleancodev2 and contribute to the code cleanup myself, as I have some spare time at the moment. Waiting for the merge until the code passes all tests has also some appeal for me.

[bosd]

On the other hand, I was considering to fork bosd:Cleancodev2 and contribute to the code cleanup myself, as I have some spare time at the moment. Waiting for the merge until the code passes all tests has also some appeal for me.

Oh Cool, I think you can pull from this repo and commit to this PR directly. (Have done something similar myself previously, but don't remember how.) Might not be the easiest option.

On second thought.. Let's merge this beast. Then follow up PR's can be smaller and easier to comprehend.

For reference, found this thread on stackoverflow to replace the f strings. Specifically the pyupgrade method looks interesting, as it may fix any possible leftovers from port to python3.

Suggestion for further cleaning:

• [ ] Fix more flake8
• [ ] Replace Except Exception to be more specific
• [ ] Remove unused var's
• [ ] Fix leftovers (if any) from port of python2 to 3
• [ ] Remove python3 port warning
• [ ] Optimize code for speed,

[m1ch]

For my understanding, only @vlachoudis and @Harvie are able to merge the PR. As already, all files are touched and still almost all files are no clean, I suggest doing further clean-up, before merging this PR. Thus, only one review of @vlachoudis, or @Harvie is required.

I completed the suggestions of @bosd

• [ ] Fix more flake8
• [ ] Replace Except Exception to be more specific
• [ ] Remove unused var's
• [ ] Fix leftovers from port of python2 to 3
• [ ] Remove Python3 port warning
• [ ] Make python3.8 the minimum compatible version
• [ ] Optimize code for speed,
• [ ] Remove commented code
• [ ] Split files that are lager than 1000 lines
• [ ] Add coding guidelines
• [ ] Add checks for PRs

@vlachoudis @Harvie please commend, if you see any problems with these suggestions

@bosd I started my work in m1ch:Cleancodev2 and created already a draft PR to bosd:Cleancodev2.

[bosd]

I started my work in m1ch:Cleancodev2 and created already a draft PR to bosd:Cleancodev2.

Cool!! So, I can fix the remaining icon issue. As I would like to deliver a "finished" job. For now my attention is needed on some other projects. In the future will make an occasional commit here. Marlin support is very high on my wishlist as well as modernizing the UI.

[rschell]

Discovered while testing my "New_control_frames" push against this one that changing "import tkExtra" to "from lib import tkExtra" in Ribbon.py results in the following error:

"TypeError: Combobox.configure() takes 1 positional argument but 2 were given"

Linux with Python 3.10.5

[m1ch]

I did further code improvements to the pull-request of @bosd (#https://github.com/bosd/bCNC/pull/1)

@vlachoudis @Harvie I did a lot of changes. Even one braking one (making python 3.8 the minimum required python version) Would you be so nice and create a testing branch from master, that I can make a PR against it? I propose this, because there are so many changes, that further testing is required.

[rschell]

Downloaded m1ch:Cleancodev2 and applied my new control frame push to it. Seams to pass the simple home and jog commands ok. Received a "version" reference error once that I can't reproduce yet. Will keep testing.

[rschell]

Found the import error. In Utils.py line 618:

    from Helpers import __version__, __date__

both variables are defined in Utils.py so there is no need for the import. Remove the line.

Only shows up when a traceback occurs.

[m1ch]

Found the import error. In Utils.py line 618:

    from Helpers import __version__, __date__

both variables are defined in Utils.py so there is no need for the import. Remove the line.

Only shows up when a traceback occurs.

@rschell good finding. I removed the line

[bosd]

@vlachoudis @Harvie The commits of @m1ch have been merged into this PR. This PR is ready for review/merge.

[m1ch]

@bosd, @Harvie FYI: I did a code-factor check on codefactor.io, and it resulted in a B.

[Harvie]

I did a code-factor check on codefactor.io, and it resulted in a B.

This is great, but the PR is so big, it's basicaly unreviewable within the free time i have available right now :-D

[Harvie]

So in order to appreciate the huge amount of work on this. i've merged this completely unreviewed and we will see if the community will be OK with the changes. We can always revert LOL.

[Harvie]

But this made virtualy all other PRs unmergeable, which i really hate. So still not sure if that was a good idea... Especialy replacing tabs with spaces (which i don't like for personal reasons) makes other PRs unnecesarily unmergeable.

[Harvie]

I've also noticed that some functionality was removed. Eg.: "onstart" command which is supposed to launch some system commands before cutting. Well codefactor might consider "unsafe" to execute string as a shell command, but users might consider that useful... That is another thing i don't like. Are there more removed features in this PR?

[bosd]

But this made virtualy all other PRs unmergeable, which i really hate.

No worries, will work with you to resolve that.

We have tested both of the forks before this PR. But was cannot guarantee it is bug free. :)

This pr is besides a code cleanup also a couple of bug fixes.

I really like it is merged as it is a good base to develop on further on.

How does the release mechanism work of pypi / pip? Users will have the choice to install this version or the old version. ( this PR included a version bump)

[bosd]

Are there more removed features in this PR?

Not that I am aware of. This is a collaboration of work together with @m1ch . Maybe he has got some thoughts on this?

[m1ch]

@Harvie Thanks! I know this PR was a lot.

Are there more removed features in this PR?

I did not remove anything on purpose. Furthermore, I don't even find the where this onstart was. If you give me a hint, I will see to it.

How does the release mechanism work of pypi / pip? Users will have the choice to install this version or the old version. ( this PR included a version bump)

Looking at this, the Git release, or the update-checker, also needs an update.

But this made virtualy all other PRs unmergeable, which i really hate. So still not sure if that was a good idea... Especialy replacing tabs with spaces (which i don't like for personal reasons) makes other PRs unnecesarily unmergeable.

If you point me to the PR in question, I can merge it to my Fork. With the right tools, the conversion from tabs to spaces is no issue.

I also plan to do some further work cleaning up the code.

As @bosd mentioned in https://github.com/bosd/bCNC/pull/1#issuecomment-1220740666 there are also some disfigurements in the GUI.

Fixing this, I rewrote big parts of the GUI. I also wanted to release the first draft version today and here it is: https://github.com/m1ch/bCNC/tree/testing

It is up-to-date with the merges you did today! What I did so far is:

• Code refactorings
• Split big modules and created packages out of them
• Made the main instances of CNC, GCode and Sender independent of the GUI, that it will be possible in future to change the language, or a theme without a restart.
• Switched all tkinter widgets, that have a ttk replacement, to ttk. Also started with the extraction of the stiles for possible theming.
• Changed the mutation of the CNC-class to a change of the global cnc-object.
• ....

I hope you like the direction where I am going with this and would love to brainstorm how to address some matters.

[bosd]

@m1ch I'm impressed with your work and all efforts you put into this!

Your screenshot shows version 0.19.14-dev. In this PR I've version bumped to 0.19.15. Remember this well. Because the version check is using a basic number compare. It couldn't handle the alphabetic chars. Thought I had tested the version check mechanism.

Fixing this, I rewrote big parts of the GUI. I also wanted to release the first draft version today and here it is: https://github.com/m1ch/bCNC/tree/testing

Awesome Your'e working on the GUI as well. Started this endevour because It was a pity people overlooked this great peace of software because of the GUI. I've learned it the hard way. Other CNC / Laser senders wer'e not stable, and run in caused dangerous situations. Then I learned to live (accept) this gui.

Did quite a research on improving the gui and small test (customtkinter). Conclusion was to go for a more ttk based approach. Like: https://github.com/rdbende/Azure-ttk-theme

Replace the icons to SVG or webfont.

Maybe some elements need re-ordering. Some inspiration from: https://www.idepartners.nl/portfolio/ux-design/machine-interface-1

Propably better to make a brainstorm somewhere else :)

[bosd]

Indeed looks like version check is against the latest github release. But now pypi is used it should check against the most recent published package there.

[m1ch]

Propably better to make a brainstorm somewhere else :)

👍️ I did not find any official developer channel for bCNC. @Harvie @bosd I can create one. Do you prefer Matrix, or Discord?

I really like it is merged as it is a good base to develop on further on.

I forgot to comment on this one. Yeah, that is the reason why I spend the work. It is more fun to contribute to open-source projects, if there is a certain level of conformity.

Did quite a research on improving the gui and small test (customtkinter). Conclusion was to go for a more ttk based approach. Like: https://github.com/rdbende/Azure-ttk-theme

Replace the icons to SVG or webfont.

Maybe some elements need re-ordering. Some inspiration from: https://www.idepartners.nl/portfolio/ux-design/machine-interface-1

My thoughts to. I will have a look.

[Harvie]

I am quite happy with github for discussions. No reason to fragment the attention across several platforms. Feel free to write a lot of messages here.

[bosd]

Fixing this, I rewrote big parts of the GUI. I also wanted to release the first draft version today and here it is: https://github.com/m1ch/bCNC/tree/testing

@m1ch Is that one ready for a draft PR as well?

---

FYI, just tried the master version of the main repo on an "older" system. Running python 3.7.3. Getting an NameError _("Open") on bfiledialog.py as _ is unrecognized. It is no longer an import.

I'm not sure how to proceed with that one. As on your testing branch you also did some work with the _ imports.

(I'm not ready yet to update python to 3.8 on that system)

[m1ch]

FYI, just tried the master version of the main repo on an "older" system. Running python 3.7.3. Getting an NameError _("Open") on bfiledialog.py as _ is unrecognized. It is no longer an import.

I'm not sure how to proceed with that one. As on your testing branch you also did some work with the _ imports.

(I'm not ready yet to update python to 3.8 on that system) @bosd This was the definition before the cleanup.

import builtins as __builtin__
__builtin__._ = gettext.translation('bCNC', os.path.join(prgpath,'locale'), fallback=True).gettext
__builtin__.N_ = lambda message: message

Python discourages to manually define builtins. I only added the import for until I found that gettext has an install method, that also adds to the builtins.

lang = gettext.translation(
__prg__,
__localespath__,
languages=[language]
)
lang.install()

You can try if the old solution works under 3.7.

Even Python 3.7 has only 9 Month till EOL. Installing 3.8 in parallel to 3.7 is no option on that computer?

---

@m1ch Is that one ready for a draft PR as well?

It is too early for a draft PR. I really try to solve some code, which I consider to be fundamental architecture problems. Because of this, the program starts, but still throws some exceptions. But for testing and playing around with styles, it should be good.

[bosd]

Even Python 3.7 has only 9 Month till EOL. Installing 3.8 in parallel to 3.7 is no option on that computer?

Yes, propably the best to upgrade that system. Will do after some dev work is finished. No further action needed from your side :)

[rschell]

Need to improve the formatting of the text in the buffer bar, suggest changing code around 2811 in bmain.py from

• self.bufferbar.setText(f"{Sender.getBufferFill(self)}%") to:
• self.bufferbar.setText(f"{Sender.getBufferFill(self):3.0f}%")

Submitted PR #1747