Skip to content

Bunch of f-string, with statements / os.path usage changes #1000

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

Merged
merged 15 commits into from
May 3, 2023

Conversation

Mikey1993
Copy link
Contributor


I have made changes to the python scripts:

  • Use f-strings where is needed, and removed where it's not
  • Converted all the os.open() statements to use the with statement for a better code etiquette
  • Made the use of os.path.normpath() and os.path.join() where is needed for better cross platform comparability and robustness

Mikey1993 added 15 commits May 3, 2023 23:38
Use f strings as it's better than using string concatenation
Change paths using `os.path.join()` and `os,path.normapth()` for a more robust and platform-independent solution + better readability.
Remove f-string where it's not needed, and add f-string where it's making more sense.
Use `os.path.join()` and `os.path.normpath` for better cross platform comparability and robustness.
Use `os.path.join()` and `os.path.normpath` for better cross platform comparability and robustness.
Use `os.path.join()` and `os.path.normpath` for better cross platform comparability and robustness.
open the contributors file with a `with()` statement instead of closing the file manually.
Use `os.path.join()` for better cross platform comparability and robustness.
Use `os.path.join()` for better cross platform comparability and robustness.
More path changes + use `with()` to open files
Use the `with` statement to open files
Use the `with` statement to open files
@Mikey1993
Copy link
Contributor Author

@marticliment I think these changes are pretty trivial, do you see any issues with merging them?

@marticliment marticliment merged commit a505538 into marticliment:main May 3, 2023
@panther7
Copy link
Contributor

panther7 commented May 5, 2023

OMG, please try before merge... @marticliment

Traceback (most recent call last):
  File "C:\Users\filip\git\WingetUI\scripts\get_contributors.py", line 38, in <module>
    contributors_filepath = os.path.normapth(os.path.join(root_dir, "wingetui/data/contributors.py"))
                            ^^^^^^^^^^^^^^^^
AttributeError: module 'ntpath' has no attribute 'normapth'. Did you mean: 'normpath'?
Traceback (most recent call last):
  File "C:\Users\filip\git\WingetUI\scripts\download_translations.py", line 32, in <module>
    os.chdir(os.path.normapth(os.path.join(root_dir, "wingetui/lang")))
             ^^^^^^^^^^^^^^^^
AttributeError: module 'ntpath' has no attribute 'normapth'. Did you mean: 'normpath'?

Btw, whats was wrong before? @Mikey1993
This is Windows only app...

@panther7
Copy link
Contributor

panther7 commented May 5, 2023

Made the use of os.path.normpath() and os.path.join() where is needed for better cross platform comparability and robustness

This is Windows only app, this commit is useless. (only one is ok, with "with" is no need file close method).

17 commits, 6 changed files, no squash, repeated commit messages... 🤦

marticliment added a commit that referenced this pull request May 5, 2023
@Mikey1993
Copy link
Contributor Author

Mikey1993 commented May 5, 2023

@marticliment ,Sorry for the mess..

  • It's still better to use os.path.join() than hard coding the paths in one string.
  • Same goes for the with for opening files.
  • Regarding the os.path.normpath(), I get what you say @panther7, but this is a good practice to use nevertheless. Also, you practically should be able to run this code on a non Windows machine as well (the python scripts), so this does fix an issue.

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.

3 participants