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

Ref: python script clean up #141

Conversation

Furrior
Copy link
Contributor

@Furrior Furrior commented Dec 9, 2023

  • Added type hints everywhere where is possible.
  • Added linter ignores to lines where the typing problem is in the used library.
  • Replaced old % based formatting with f-strings
  • Renamed a lot of vars
  • Some minor things.

Still need some help with one function that I dont fully understand. Looks like some lua version parser, but I have never actually worked with lua, so need ideas how to rename stuff there.

Also I did NOT test the code. YOLO

@Furrior
Copy link
Contributor Author

Furrior commented Dec 9, 2023

@aaptel Could you take a look?

@Furrior
Copy link
Contributor Author

Furrior commented Dec 9, 2023

My main goal was to pass pycharm linter and pylance without any warnings

@aaptel
Copy link
Collaborator

aaptel commented Dec 9, 2023

Hmm I'll be honnest, I appreciate the effort and time to do this, but I don't like the changes so much. 😕
I've reviewed the code in case you really want to push this (there are some good things too 👍 ).
But keep in mind this is a quick&small script for developpers, I think time would be best invested elsewhere in the project.

  • All the # ignore type annotations are just noise.
  • Type annotation at the function prototype is ok, but anywhere else is noisy and pedantic.
  • Imports should be sorted by features, alphabetical is pointless.
  • Short variable names can be easier to read than longer ones depending on the context.
  • Wrapping some of the long lines makes them less readable because I've tried to keep the powershell commands 1 per line and aligned.

And please test the code. 🙏 I care more that it works than it looks pretty.

@Ismoh
Copy link
Owner

Ismoh commented Dec 9, 2023

@Furrior statement?
merge?
apply suggestions?
or close?

@Furrior
Copy link
Contributor Author

Furrior commented Dec 9, 2023

I ll apply suggestions soon

@Furrior
Copy link
Contributor Author

Furrior commented Dec 10, 2023

  • Tested the code 😅
  • Removed some extra stuff to the state where pycharm finds no erorrs, but pylance does.
  • Left type hints in function bodies only where its necessary(the lib typing is this bad that pycharm cant determine what stuff some function returns)
  • Imho the var names are much better this way, because I felt myself like renaming stuff after decompiling smth to C.

I agree with the fact that this script is not of primary importance, but it was barely readable for me in its original state. Also the branch name is "if-you-do-it-quickndirty-your-code-quality-will-suffer", so...

Ready for a merge if Ismoh doesnt mind a little bit of excessive pedantry.

@Ismoh
Copy link
Owner

Ismoh commented Dec 10, 2023

Tbh I don't like

excessive pedantry

!

I pass on @aaptel. Please make a decission. It's your code.

@Furrior
Copy link
Contributor Author

Furrior commented Dec 10, 2023

My point about type hints - if ide cant identify what is stored in the var, then it needs a type hint

@Furrior
Copy link
Contributor Author

Furrior commented Jan 15, 2024

I guess it means no

@Furrior Furrior closed this Jan 15, 2024
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