refactor: python doc script classes #301
Conversation
jandom
force-pushed
the
jandom/refactor/document-classes
branch
from
ba7b387
to
1153df5
Compare
jandom
changed the base branch from
jandom/feat/add-snapshot-tests-with-syrup
to
develop
jandom
changed the title
jandom
changed the base branch from
develop
to
jandom/feat/add-snapshot-tests-with-syrup
jandom
force-pushed
the
jandom/refactor/document-classes
branch
from
43fc301
to
cbe7e4a
Compare
jandom
changed the base branch from
jandom/feat/add-snapshot-tests-with-syrup
to
develop
There was a problem hiding this comment.
A partial review to start off with -- this looks fantastic @jandom! Everything I've seen looks a thousand times more elegant and easy to read and maintain than what was there before; I'm pretty sure all of my comments are picking up stuff that you hadn't yet changed from the original code.
|
The one thing that i'm leaving out of this PR is changes to a few files:
Also, any flake8 enforcement will be left for later (but I have used it for local development to weed out a few simple mistakes). |
jandom
changed the base branch from
develop
to
jandom/feat/add-snapshot-tests-with-syrup
|
Some CI/CD is mysteriously broken... will investigate Meanwhile, this PR remains broken on the syrup snapshot testing PR: these tests ensure that the refactor doesn't change the behavior of the code, so it's important that we merge that first |
|
re: syrup snapshot testing PR Apologies, I realise I'm a blocker here. I'm unlikely to have the chance to look at things until the weekend, it's one of those where if I'm for now still in charge of releases (we will see what happens), then I would like to have a review of how this changes the release process. |
|
No worries and let’s take all the time here. These PRs here are just demos, and not urgent at all |
jandom
force-pushed
the
jandom/refactor/document-classes
branch
from
488692c
to
00d2d6d
Compare
jandom
force-pushed
the
jandom/feat/add-snapshot-tests-with-syrup
branch
from
119e680
to
b716c8a
Compare
migrate gen_topology_groupmethods.py migrate gen_topologyattr_defaults.py migrate FormatOverview migrate CoordinateReaders migrate gen_format_overview_classes migrate TopologyParsers migrate TopologyAttrs migrate ConnectivityAttrs remove dead code, never executed remove all class attributes all non-strict mypy checks pass, mypy in pre-commit first nice simplification another nice simplification more simplifications further cleanups further trimmings continue to refactor update base.py harmonize to use private functions simpler code architecture more refactoring mypy strict mode on some files finish mypy changes flake8 on everything Update doc/source/scripts/base.py Co-authored-by: Lily Wang <[email protected]> Update doc/source/scripts/gen_format_overview_classes.py Co-authored-by: Lily Wang <[email protected]> Update doc/source/scripts/gen_format_overview_classes.py Co-authored-by: Lily Wang <[email protected]> Update doc/source/scripts/gen_format_overview_classes.py Co-authored-by: Lily Wang <[email protected]> Update doc/source/scripts/gen_format_overview_classes.py Co-authored-by: Lily Wang <[email protected]> review comments from Lily fixing mypy
jandom
force-pushed
the
jandom/refactor/document-classes
branch
from
00d2d6d
to
3269982
Compare
Co-authored-by: Lily Wang <[email protected]>
Co-authored-by: Lily Wang <[email protected]>
There was a problem hiding this comment.
Finished looking over the other scripts -- just some minor comments, but otherwise everything looks great!! Thank you @jandom :)
Co-authored-by: Lily Wang <[email protected]>
Co-authored-by: Lily Wang <[email protected]>
Co-authored-by: Lily Wang <[email protected]>
Co-authored-by: Lily Wang <[email protected]>
Co-authored-by: Lily Wang <[email protected]>
|
Thank you @lilyminium – those were some really great suggestions, merged them all and tweaked the tests to match! |
There was a problem hiding this comment.
Thanks @jandom -- this overhaul looks fantastic! Looking forward to seeing them in the main repo :D
|
Thank you for the kind words @lilyminium that's really nice – I'm still not completely happy with it, it could be much simpler. But yeah my plan was to get some folks familiar with this toolchain and then slowly start hammering away at the core repo ;-) Maybe I'll add flake8 here and wrap up – also Irfan will probably have more feedback |
|
This should see the light of day soon, merged develop into this feature branch and hopefully everything will pass. |
This is a slow grind but having the tests there protects me from a lot of simple mistakes.
It's still a bit early, I want to make the TableWriter as dumb as possible. Right now it has too many responsibilities and the code structure is very confusing (the technical term is "inheritance hell")
The worst offender is definitely gen_topologyparser_attrs.py the only file I haven't migrated so far