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

Dev: Cleanup and pep8 #213

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

madeddy
Copy link
Contributor

@madeddy madeddy commented Apr 1, 2024

Not much in terms of code changes beyond removal of a few unused imports, vars, a leftover comment (init.py L489) and some added parens for code stacking. However a lot of formatting.

@CensoredUsername
Copy link
Owner

Huh, I never knew of the pep8 standard of at least 2 spaces before an inline comment.

decompiler/__init__.py Outdated Show resolved Hide resolved
unrpyc.py Outdated Show resolved Hide resolved
@CensoredUsername
Copy link
Owner

Went through it all, there's only 2 things I'm not really a fan of that I marked above. Thanks!

@madeddy
Copy link
Contributor Author

madeddy commented Apr 4, 2024

Huh, I never knew of the pep8 standard of at least 2 spaces before an inline comment.

I was also surprised at the first time i read it. Among others. Personally i don't care if its 1, 2 or 3 but more or none spaces is imho counterproductive.
As said, the linter is useful IMO, but it gives just the options to block the error code in question generally, turn off lint for the whole file or with 'NOQA' at every line with a error i don't care about. A section 'NOQA' would be good.

BTW, fun fact: unrpyc code breaks NEARLY every line spacing rule:
E301: expected 1 blank line, found 0
E302: expected 2 blank lines, found 0
E303: too many blank lines (3)
E304: blank lines found after function decorator <- This one not
E305: expected 2 blank lines after end of function or class
E306: expected 1 blank line before a nested definition

😁 You have your own system with the code section comments e.g. # API or # Machinery so i don't touched this. I do not want to be a grammar nazi, but some rules make imho sense. At least to avoid chaos code.

@madeddy madeddy force-pushed the dev_cleanup_and_pep8 branch from 162e78f to 4fc9c31 Compare April 4, 2024 19:16
madeddy added 5 commits April 4, 2024 21:21
- membership should be tested with "(not) in"
- remove unused vars in exceptions
- correct whitespace around operators, slices
- line break before keywords and operators
- max-line-length formating
- whitespace amount for inline comments
- code stacking for better readability
@madeddy madeddy force-pushed the dev_cleanup_and_pep8 branch from 4fc9c31 to d7ad578 Compare April 4, 2024 19:21
- wrong operator changes
- adjust some format choices and line lengths
@madeddy madeddy force-pushed the dev_cleanup_and_pep8 branch from 7f1d0e7 to e8c1619 Compare April 4, 2024 19:59
@madeddy
Copy link
Contributor Author

madeddy commented Apr 4, 2024

So, i made some changes according your comments. If you want other corrections just tell. It's no problem.

@CensoredUsername
Copy link
Owner

CensoredUsername commented Apr 4, 2024

BTW, fun fact: unrpyc code breaks NEARLY every line spacing rule:

TBH, those things are a bit too rigid at times. There's a lot I do agree with in general (empty lines after functions / classes etc) but sometimes you want to group some items together (or at least, separate them from some others clearly) and then things like too many empty lines between are just annoying.

I think it's fine now, can you change it from a work in progress?

@madeddy madeddy marked this pull request as ready for review April 4, 2024 23:17
@madeddy
Copy link
Contributor Author

madeddy commented Apr 4, 2024

sometimes you want to group some items together (or at least, separate them from some others clearly)

The same. I like to do this for code parts but also imports. And then the formatter mucks it up.

change it from a work in progress

Will do. I thought a draft brings some advantages but i noticed none.

@CensoredUsername
Copy link
Owner

Will do. I thought a draft brings some advantages but i noticed none.

It just disallows me from merging it in the web UI.

@CensoredUsername CensoredUsername merged commit f6c99d3 into CensoredUsername:dev Apr 16, 2024
2 checks passed
@madeddy madeddy deleted the dev_cleanup_and_pep8 branch April 18, 2024 20:15
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