refactor: python doc script classes

MDAnalysis / UserGuide

User Guide for MDAnalysis

https://userguide.mdanalysis.org

22 stars 33 forks source link

refactor: python doc script classes #301

Closed jandom closed 12 months ago

jandom commented 1 year ago

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

jandom commented 1 year ago

The one thing that i'm leaving out of this PR is changes to a few files:

conf.py
core.py
clean_example_notebooks.py
update_json_stubs_sitemap.py

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 commented 1 year ago

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

https://github.com/MDAnalysis/UserGuide/pull/285

IAlibay commented 1 year ago

@jandom

Some CI/CD is mysteriously broken... will investigate

This is already fixed in #304

IAlibay commented 1 year ago

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.

jandom commented 1 year ago

No worries and let’s take all the time here. These PRs here are just demos, and not urgent at all

jandom commented 1 year ago

Thank you @lilyminium – those were some really great suggestions, merged them all and tweaked the tests to match!

jandom commented 1 year ago

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

jandom commented 12 months ago

This should see the light of day soon, merged develop into this feature branch and hopefully everything will pass.