tests: lint wycheproof’s python script #1279

pull RandomLattice wants to merge 1 commits into bitcoin-core:master from RandomLattice:lint changing 1 files +22 −21
  1. RandomLattice commented at 11:40 am on April 14, 2023: contributor
    This PR lints tests_wycheproof_generate.py according to bitcoin’s python linting scripts. This is a follow-up to PR #1245.
  2. real-or-random cross-referenced this on Apr 14, 2023 from issue ci: bring bitcoin's .py linter into secp256k1 by RandomLattice
  3. sipa commented at 12:46 pm on April 14, 2023: contributor

    Some of the changes here are clear improvements (dropping unnecessary imports, for example), and others probably do make the code more “standard” Python, so I’m inclined to take it - not because it satisfies some linter but just because it improves the code.

    But if that’s the motivation, perhaps we can go a bit further, and see what other linters say. pylint in addition complains about:

    • Too long lines (meh)
    • Variables whose names are too short (meh)
    • Constants that are not UPPER_CASE styled (mildly in favor)
    • Using f-strings for formatting instead of using string manipulation (definitely more readable, I think)
    • On line 59 and 72, “Consider iterating the dictionary directly instead of calling .keys()” (seems reasonable).

    WDYT?

  4. real-or-random commented at 1:16 pm on April 14, 2023: contributor

    @sipa Makes sense.

    But if your “disable linting” in https://github.com/bitcoin/bitcoin/pull/27445 is just a WIP commit you don’t want to have there right now, I’m also happy to merge this PR here quickly. We know that the current changes make Core’s linter happy. We should then address pylint stuff in a separate PR.

  5. sipa commented at 1:18 pm on April 14, 2023: contributor

    I don’t think we should make any further changes in this repo.

    I’ll update the PR there to disable linting of the subtree in a less WIPy way.

  6. real-or-random commented at 1:21 pm on April 14, 2023: contributor
    Ok, just sounds good! Either approach is fine with me.
  7. RandomLattice commented at 1:24 pm on April 14, 2023: contributor
    Sounds like we can update this PR to make pylint happier (addressing the points that @sipa raised), but let’s decouple this PR from the ongoing secp256k1 bump in bitcoin. Does this sound good?
  8. sipa commented at 1:25 pm on April 14, 2023: contributor
  9. RandomLattice force-pushed on Apr 14, 2023
  10. RandomLattice commented at 4:38 pm on April 14, 2023: contributor

    I added some pylint suggestions but not all. I took:

    • Using f-strings for formatting (makes it more readable)
    • Iterate the dictionary instead of calling .keys()

    I did not take:

    • Lines too long
    • Variable names are too short
    • UPPER_CASE style for variable names, since those aren’t constant

    These changes do not change the input/output behavior of the script.

  11. tests: lint wycheproof's python script
    This PR lints tests_wycheproof_generate.py according to pylint.
    This is a follow-up to PR #1245.
    
    Co-authored-by: Sean Andersen <6730974+andozw@users.noreply.github.com>
    35ada3b954
  12. in tools/tests_wycheproof_generate.py:84 in ecbbdbbc2e outdated
    88-                offset_sig,
    89-                sig_size,
    90-                expected_verify) + " },\n"
    91-        if new_msg: offset_msg_running += msg_size
    92-        if new_pk: offset_pk_running += 65
    93+        out += "  {" + f"{pk_offset}, {msg_offset}, {msg_size}, {offset_sig}, {sig_size}, {expected_verify}" + " },\n"
    


    sipa commented at 4:40 pm on April 14, 2023:
    The " {" and " },\n" parts can be part of the f-string too.

    RandomLattice commented at 4:57 pm on April 14, 2023:
    👍 addressed, thanks. The single { needs escaping into {{ inside a f-string but probably still readable
  13. RandomLattice force-pushed on Apr 14, 2023
  14. real-or-random approved
  15. real-or-random commented at 12:09 pm on April 19, 2023: contributor
    utACK 35ada3b954ccc6c54628fb3bcc0365d176297019
  16. sipa commented at 1:48 pm on April 19, 2023: contributor
    utACK 35ada3b954ccc6c54628fb3bcc0365d176297019
  17. real-or-random merged this on Apr 19, 2023
  18. real-or-random closed this on Apr 19, 2023

  19. sipa referenced this in commit b4eb644b6c on May 12, 2023
  20. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
  21. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  22. sipa referenced this in commit 901336eee7 on Jun 21, 2023
  23. vmta referenced this in commit 8f03457eed on Jul 1, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-23 07:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me