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

Cleancodev2 #1

Merged
merged 20 commits into from
Aug 25, 2022
Merged

Cleancodev2 #1

merged 20 commits into from
Aug 25, 2022

Conversation

m1ch
Copy link

@m1ch m1ch commented Jul 12, 2022

I started to do further improvements of the code. It will take me some days till I'm done.
As there are still a lot of files that need an update, I would prefer to first merge it in your branch.

m1ch added 11 commits July 11, 2022 17:41
pyupgrade --py38-plus
flynt
black
flake8

Manual:
clean imports (Remove imports from Python2)
Update lines exeeding 80 chars
Update not automatic fixed %-formated strings
pyupgrade --py38-plus
flynt
black
flake8

Manual:
Update lines exeeding 80 chars
Update not automatic fixed %-formated strings
Changes:
Automatic:
pyupgrade --py38-plus
flynt
black
flake8

Manual:
clean imports (Remove imports from Python2)
Update lines exceeding 80 chars
Update not automatic fixed %-formatted strings
clean imports (Remove imports from Python2)
Update lines exceeding 80 chars
Update not automatic fixed %-formatted strings

remove all __future__ imports
remove import *
resolve circular imports (still work required)

renamed __main__.py to bmain.py and created new __main__.py. This was necessary
due to "E402 - module level import not at top of file"

flake8 does not support builtins. Thus and also for readability I changed
builtins._ and .N_ to Utils methods and add `from Utils import _, N_` to
required files.

A001 - variable "" is shadowing a python builtin
A002 - argument "" is shadowing a python builtin

F401 - imported but unused
F841 - local variable is assigned to but never used

E201 - whitespace after '('
E202 - whitespace before ')'
E203 - whitespace before ':'
E402 - module level import not at top of file
E722 - do not use bare 'except'

N805 - first argument of a method should be named 'self'
W601 - .has_key() is deprecated, use 'in'

Final flake8 test:
    flake8 --max-doc-length 1000 --statistics --ignore A003,E501,E713,E741,N801,N802,N803,N806,N815,N816,W503 ./bCNC > flake8.out
with one error remaining:
    ./bCNC/Utils.py:530:26: F821 undefined name 'self'
Remove commented code
2     E302 expected 2 blank lines, found 1
1     E303 too many blank lines (2)
1     E305 expected 2 blank lines after class or function definition, found 1
1     E713 test for membership should be 'not in'
9     E741 ambiguous variable name 'O'
1     F821 undefined name 'self'
@m1ch m1ch marked this pull request as ready for review July 26, 2022 10:42
@m1ch
Copy link
Author

m1ch commented Jul 26, 2022

I think I could improve the overall code quality quite a bit.

There are still lots of flake8 errors to be fixed:
42 A003 class attribute "vars" is shadowing a python builtin
429 E501 line too long (80 > 79 characters)
17 N801 class name 'abcDROFrame' should use CapWords convention
940 N802 function name 'getValue' should be lowercase
108 N803 argument name 'helixBottom' should be lowercase
814 N806 variable 'A' in function should be lowercase
12 N815 variable 'drillPolicy' in class scope should not be mixedCase
21 N816 variable 'iniSystem' in global scope should not be mixedCase

@m1ch
Copy link
Author

m1ch commented Jul 26, 2022

I also had to add two new files:

  • bmain.py ... major part from main.py because of "E402 module level import not at top of file"
  • Helpers.py ... some functions from Utils.py because of circular imports

@bosd
Copy link
Owner

bosd commented Jul 27, 2022

I also had to add two new files:

  • bmain.py ... major part from main.py because of "E402 module level import not at top of file"

Somehow, I did'nt get a notification for this PR. But I'm here now.

I struggled quite a bit with the imports. The pre-commit script I had use moved all the imports in the right order.. But it ended up breaking the code.
After some debugging, I moved back to old position and code to save time.
Glad you're on it!

bCNC/bmain.py Outdated Show resolved Hide resolved
bCNC/bmain.py Outdated Show resolved Hide resolved
vlachoudis#1714 (comment)
bosd#1 (comment)
bosd#1 (comment)

Also changed application to be inherited from tkinter.Tk, instead of
tkinter.Toplevel. This change also fixes the issue, that the windowname
is also displayed in the dock.

Further I moved bmain.main functionality back to __main__.main.
@bosd
Copy link
Owner

bosd commented Jul 29, 2022

Just realized that these big changes in source code also affect the localization.
Strings have moved to other places in the source code. Strings with %s are converted to f strings. Propably the translation files need to be re-generated.

EDIT: Just realized you took already care of that! Awesome!! 🎉 ✨

@m1ch
Copy link
Author

m1ch commented Aug 1, 2022

Translation was not working in my last commit. Fixed it now

@bosd
Copy link
Owner

bosd commented Aug 11, 2022

Is this one ready for review?

@m1ch
Copy link
Author

m1ch commented Aug 12, 2022

Yes. This PR is ready.
I started already some further work based on this PR, but it is far more than clean up, thus I will make a separate PR against main, once this is accepted.

Copy link
Owner

@bosd bosd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review on windows 10, python 3.10.
Test failling to load main window

bCNC/__main__.py Outdated Show resolved Hide resolved
bCNC/__main__.py Outdated Show resolved Hide resolved
@bosd
Copy link
Owner

bosd commented Aug 19, 2022

@m1ch Thanks, main window is loading now!
Is it intentional that after loading the main window, also the "Check for updates" window is opened?

unrelated to this pr, found this irregularity.
The colors in the dropdown in the "cam" ribbon are different then in the other menus.
(The cam menu ignores the users color setting)
image

Other random dropdown
image

Quick DXF file loading
Loaded a DXF file, just to test gui. It was saved in autocad 2000 format.
But bCNC could not make anything out of it..
This is just a random test with this file. Not sure if it is related to the code changes.
Will need further testing.
image

@bosd
Copy link
Owner

bosd commented Aug 19, 2022

I've totally missed the feedback in the dos window.
image

@bosd
Copy link
Owner

bosd commented Aug 25, 2022

Another functional test. This PR is OK now!
Will open new "issues" in main bCNC repo for new found items.
Like the DXF not handled properly. Maybe the DXF engine/parser needs updating.

@bosd bosd merged commit b80c8f8 into bosd:Cleancodev2 Aug 25, 2022
Harvie pushed a commit to vlachoudis/bCNC that referenced this pull request Sep 1, 2022
* Automatic:
pyupgrade --py38-plus
flynt
black
flake8

Manual:
clean imports (Remove imports from Python2)
Update lines exeeding 80 chars
Update not automatic fixed %-formated strings

* Automatic:
pyupgrade --py38-plus
flynt
black
flake8

Manual:
Update lines exeeding 80 chars
Update not automatic fixed %-formated strings

* Draft: intermediate commit

Changes:
Automatic:
pyupgrade --py38-plus
flynt
black
flake8

Manual:
clean imports (Remove imports from Python2)
Update lines exceeding 80 chars
Update not automatic fixed %-formatted strings

* Draft: intermediate commit

clean imports (Remove imports from Python2)
Update lines exceeding 80 chars
Update not automatic fixed %-formatted strings

remove all __future__ imports
remove import *
resolve circular imports (still work required)

renamed __main__.py to bmain.py and created new __main__.py. This was necessary
due to "E402 - module level import not at top of file"

flake8 does not support builtins. Thus and also for readability I changed
builtins._ and .N_ to Utils methods and add `from Utils import _, N_` to
required files.

A001 - variable "" is shadowing a python builtin
A002 - argument "" is shadowing a python builtin

F401 - imported but unused
F841 - local variable is assigned to but never used

E201 - whitespace after '('
E202 - whitespace before ')'
E203 - whitespace before ':'
E402 - module level import not at top of file
E722 - do not use bare 'except'

N805 - first argument of a method should be named 'self'
W601 - .has_key() is deprecated, use 'in'

Final flake8 test:
    flake8 --max-doc-length 1000 --statistics --ignore A003,E501,E713,E741,N801,N802,N803,N806,N815,N816,W503 ./bCNC > flake8.out
with one error remaining:
    ./bCNC/Utils.py:530:26: F821 undefined name 'self'

* Fix deleted import

* Move pot file under locale

* Draft: Mainly change for translation _() strings to the "".format()
some smaller fixed

* Draft: updated one %-formated string

* Replace remaining \t characters
Remove commented code

* flake8 Fixes:
2     E302 expected 2 blank lines, found 1
1     E303 too many blank lines (2)
1     E305 expected 2 blank lines after class or function definition, found 1
1     E713 test for membership should be 'not in'
9     E741 ambiguous variable name 'O'
1     F821 undefined name 'self'

* improved some imports

* Update CHANGELOG.md

* Update CHANGELOG.md

* improve imports

* Fixed:
#1714 (comment)
bosd#1 (comment)
bosd#1 (comment)

Also changed application to be inherited from tkinter.Tk, instead of
tkinter.Toplevel. This change also fixes the issue, that the windowname
is also displayed in the dock.

Further I moved bmain.main functionality back to __main__.main.

* Fixed translation issue

* small fixes for bosd#1

* small fixes for bosd#1

* Bugfixes from string type conversions
rar8000 pushed a commit to rar8000/bCNC that referenced this pull request Jul 21, 2023
* Automatic:
pyupgrade --py38-plus
flynt
black
flake8

Manual:
clean imports (Remove imports from Python2)
Update lines exeeding 80 chars
Update not automatic fixed %-formated strings

* Automatic:
pyupgrade --py38-plus
flynt
black
flake8

Manual:
Update lines exeeding 80 chars
Update not automatic fixed %-formated strings

* Draft: intermediate commit

Changes:
Automatic:
pyupgrade --py38-plus
flynt
black
flake8

Manual:
clean imports (Remove imports from Python2)
Update lines exceeding 80 chars
Update not automatic fixed %-formatted strings

* Draft: intermediate commit

clean imports (Remove imports from Python2)
Update lines exceeding 80 chars
Update not automatic fixed %-formatted strings

remove all __future__ imports
remove import *
resolve circular imports (still work required)

renamed __main__.py to bmain.py and created new __main__.py. This was necessary
due to "E402 - module level import not at top of file"

flake8 does not support builtins. Thus and also for readability I changed
builtins._ and .N_ to Utils methods and add `from Utils import _, N_` to
required files.

A001 - variable "" is shadowing a python builtin
A002 - argument "" is shadowing a python builtin

F401 - imported but unused
F841 - local variable is assigned to but never used

E201 - whitespace after '('
E202 - whitespace before ')'
E203 - whitespace before ':'
E402 - module level import not at top of file
E722 - do not use bare 'except'

N805 - first argument of a method should be named 'self'
W601 - .has_key() is deprecated, use 'in'

Final flake8 test:
    flake8 --max-doc-length 1000 --statistics --ignore A003,E501,E713,E741,N801,N802,N803,N806,N815,N816,W503 ./bCNC > flake8.out
with one error remaining:
    ./bCNC/Utils.py:530:26: F821 undefined name 'self'

* Fix deleted import

* Move pot file under locale

* Draft: Mainly change for translation _() strings to the "".format()
some smaller fixed

* Draft: updated one %-formated string

* Replace remaining \t characters
Remove commented code

* flake8 Fixes:
2     E302 expected 2 blank lines, found 1
1     E303 too many blank lines (2)
1     E305 expected 2 blank lines after class or function definition, found 1
1     E713 test for membership should be 'not in'
9     E741 ambiguous variable name 'O'
1     F821 undefined name 'self'

* improved some imports

* Update CHANGELOG.md

* Update CHANGELOG.md

* improve imports

* Fixed:
vlachoudis#1714 (comment)
bosd#1 (comment)
bosd#1 (comment)

Also changed application to be inherited from tkinter.Tk, instead of
tkinter.Toplevel. This change also fixes the issue, that the windowname
is also displayed in the dock.

Further I moved bmain.main functionality back to __main__.main.

* Fixed translation issue

* small fixes for bosd#1

* small fixes for bosd#1

* Bugfixes from string type conversions
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