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

Code refactor, meta stuff #1028

Open
7 tasks
bswck opened this issue Jul 24, 2023 · 3 comments
Open
7 tasks

Code refactor, meta stuff #1028

bswck opened this issue Jul 24, 2023 · 3 comments

Comments

@bswck
Copy link

bswck commented Jul 24, 2023

  • Fix inconsistent string formatting (both .format() and f-string methods are used)
  • Refactor & optimize algorithms (syzygy, gaviota)
  • Shorten maximum line length to 99 (thank later)
  • Migrate to PEP 604 X | Y type syntax due to Drop support for Python 3.9 #564
  • Add a requirements.txt file (or even better - migrate from setuptools to poetry)
  • Fix 517 issues reported by Ruff, possibly add a Ruff spec file or add a proper section in pyproject.toml if we're into poetry
  • Bump pylint rating, my initial goal: 9.00/10, current rating: 7.64/10

Some algorithms do need logic refactoring, for instance

for i in range(5):
j = 0
s = 0
while j < 6:
PAWNIDX[i][j] = s
s += 1 if i == 0 else binom(PTWIST[INVFLAP[j]], i)
j += 1
PFACTOR[i][0] = s
s = 0
while j < 12:
PAWNIDX[i][j] = s
s += 1 if i == 0 else binom(PTWIST[INVFLAP[j]], i)
j += 1
PFACTOR[i][1] = s
s = 0
while j < 18:
PAWNIDX[i][j] = s
s += 1 if i == 0 else binom(PTWIST[INVFLAP[j]], i)
j += 1
PFACTOR[i][2] = s
s = 0
while j < 24:
PAWNIDX[i][j] = s
s += 1 if i == 0 else binom(PTWIST[INVFLAP[j]], i)
j += 1
PFACTOR[i][3] = s

which could simply be truncated into

for i in range(5):
    j = 0
    a = PFACTOR[i]
    p = PAWNIDX[i]

    for k in range(4):
        s = 0
        t = 6 * (k+1)
        while j < t:
            p[j] = s
            s += 1 if i == 0 else binom(PTWIST[INVFLAP[j]], i)
            j += 1
        a[k] = s

I'll work on those enhancements soon. Please let me know what you think about migrating to poetry and maybe possibly migrating tests to pytest to make them easier to read and work with. @niklasf

@bswck bswck changed the title Code refactor Code refactor, meta stuff Jul 24, 2023
@niklasf
Copy link
Owner

niklasf commented Jul 25, 2023

Hi, some thoughts about these:

  • (1), (2), (3): Sure, if the new code would be more readable. For (2), the code was ported line by line from a C original. It has since diverged, so I'd be fine with giving up the 1:1 correspondence and make it look more like Python code.
  • (4) will only be merged after Python 3.9 is past its end of life.
  • (5): There are no dependencies :)
  • (6), (7): Sure, if the findings make sense.

For pytest, at a glance I don't see how it would improve dx for this project. Do you have any particular features in mind?

@kraktus
Copy link
Contributor

kraktus commented Aug 5, 2023

About (4), I think as long as you import from __future__ importannotations, you should be able to use the new syntax just fine, we use it on berserk and support 3.8

@bswck
Copy link
Author

bswck commented Apr 10, 2024

It can actually work even for type hints evaluated at runtime since https://github.com/alexmojaki/eval_type_backport.

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

No branches or pull requests

3 participants