Opened 3 weeks ago

Last modified 3 weeks ago

#30359 new enhancement

Stem PEP8 compliant

Reported by: 0xrichard Owned by: atagar
Priority: Low Milestone:
Component: Core Tor/Stem Version:
Severity: Minor Keywords: dev
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Make the project PEP8 compliant.

https://github.com/rj76/stem/pull/1

Child Tickets

Change History (8)

comment:1 Changed 3 weeks ago by atagar

Hi 0xrichard, thanks for the patch! In this case though I kinda wish you'd talked with me before investing your time. Stem runs pycodestyle as part of its tests, and already complies with PEP8 in most regards. The ways in which it differs (for instance, two space indentation) is intentional...

https://gitweb.torproject.org/stem.git/tree/test/settings.cfg#n95

If you'd care to push for us to be more compliant that's fine, but the approach we should take is...

  1. Decide which of the above PEP8 compliance rules you feel strongly that we should follow.
  1. File a ticket to discuss why you think we should change it.
  1. Once we've established a consensus on following the rule remove its ignore configuration from the file mentioned above. Stem's tests should now cite all the spots where we don't comply with it.
  1. Make the adjustments (like your patch does) to correct the compliance issues.

Does that make sense?

comment:2 Changed 3 weeks ago by 0xrichard

... before investing your time.

I had a great day, found some things I would've done differently, like putting stuff in init.py

Decide which of the above PEP8 compliance rules you feel strongly that we should follow.

[flake8]
ignore = E501,W504,F811,F821,W605,E402,F401,F405,E131,W503,E265,E999,F403

File a ticket to discuss why you think we should change it.

Just did

Once we've established a consensus on following the rule remove its ignore configuration from the file mentioned above. Stem's tests should now cite all the spots where we don't comply with it.

That's up to you, you tell me. I got time :)

Make the adjustments (like your patch does) to correct the compliance issues.

I'd be happy to

comment:3 Changed 3 weeks ago by 0xrichard

and the circular imports :(

comment:4 Changed 3 weeks ago by 0xrichard

and switch to pytest with fixtures

comment:5 Changed 3 weeks ago by atagar

I had a great day, found some things I would've done differently

Fantastic! Glad you found this fruitful. :)

File a ticket to discuss why you think we should change it.

Just did

Yup! But we still need to discuss both what stylistic aspects you think we should change and why. Stem should be conformant with PEP8 except in the following respects...

  • Two space indentation rather than four.
  • Bare except clauses.
  • Space between keywords and arguments.

Which of these do you strongly feel we should change and why?

and the circular imports :(

If you make a separate commit with this and repro steps for triggering the circular dependency bug I'd be happy to chat.

and switch to pytest with fixtures

What benefit will that provide over what we presently have?

comment:6 Changed 3 weeks ago by 0xrichard

and this

https://gitweb.torproject.org/stem.git/tree/stem/socket.py#n392

haven't seen that in like 20 years

comment:7 Changed 3 weeks ago by 0xrichard

Which of these do you strongly feel we should change and why?

all, see:

inore = E501,W504,F811,F821,W605,E402,F401,F405,E131,W503,E265,E999,F403

If you make a separate commit with this and repro steps for triggering the circular dependency bug I'd be happy to chat.

don't put stuff in init

What benefit will that provide over what we presently have?

flexibility

I recently rewrote +/- 400 django unit tests to pytest, and yes, it's worth it

comment:8 Changed 3 weeks ago by atagar

all, see

Gotcha. And why do you feel strongly we should change this?

don't put stuff in init

Do you have a use case where this causes any problems? Putting code in init is common, and done by lots of projects.

flexibility... and yes, it's worth it

Sorry, but this isn't terribly persuasive. Personally I think our present tests provide both great coverage and readable output but if you have a practical reason to think pytest would be better I'm all ears.

Note: See TracTickets for help on using tickets.