tests: lint wycheproof’s python script #1279

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

   @RandomLattice SGTM
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
24. jonasnick cross-referenced this on Jul 24, 2023 from issue Upstream PRs 1268, 1276, 1267, 1265, 1230, 1279, 1273, 1263, 1231, 1285, 1283, 1205, 1286, 1275, 1234, 1239, 1240, 1284, 1277, 1289, 1270, 1296, 1301, 1299, 1066, 1300, 1292, 1305, 1303, 1133, 1306, 1207, 1304, 1307, 1311, 1309, 1312 by jonasnick

Contributors
RandomLattice sipa real-or-random