Skip to content
Snippets Groups Projects

Stem PEP8 compliant

    • Closed Issue created by Trac @tracbot

    Make the project PEP8 compliant.

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

    Trac:
    Username: 0xrichard

    To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

    Child items 0

    • No child items are currently assigned. Use child items to break down this issue into smaller parts.

    Activity

    • Trac added component::archived/stem dev owner::atagar priority::low reporter::0xrichard resolution::user disappeared severity::minor status::closed type::enhancement labels

      added component::archived/stem dev owner::atagar priority::low reporter::0xrichard resolution::user disappeared severity::minor status::closed type::enhancement labels

    • A
      Damian Johnson @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.

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

      3. 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.

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

      Does that make sense?

    • Trac
      Trac @tracbot ·
      Author

      ... 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

      Trac:
      Username: 0xrichard

    • Trac
      Trac @tracbot ·
      Author

      and the circular imports :(

      Trac:
      Username: 0xrichard

    • Trac
      Trac @tracbot ·
      Author

      and switch to pytest with fixtures

      Trac:
      Username: 0xrichard

    • A
      Damian Johnson @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?

    • Trac
      Trac @tracbot ·
      Author

      and this

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

      haven't seen that in like 20 years

      Trac:
      Username: 0xrichard

    • Trac
      Trac @tracbot ·
      Author

      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

      Trac:
      Username: 0xrichard

    • A
      Damian Johnson @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.

    • A
      Damian Johnson @atagar ·

      Trac:
      Status: new to closed
      Resolution: N/A to user disappeared

    • Trac closed

      closed

    Please register or sign in to reply