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

Self return type on from_dict methods #3702

Merged
merged 2 commits into from
Mar 22, 2024
Merged

Self return type on from_dict methods #3702

merged 2 commits into from
Mar 22, 2024

Conversation

janosh
Copy link
Member

@janosh janosh commented Mar 22, 2024

follow up to @DanielYang59's start in #3694 of adding Self return types on from_dict methods. far from complete. should be set on from_str and other from_... methods as well

def from_dict(cls, dct: dict) -> Self:
@janosh janosh added the types Type all the things label Mar 22, 2024
@janosh janosh enabled auto-merge (squash) March 22, 2024 18:26
@janosh janosh merged commit 81809eb into master Mar 22, 2024
22 checks passed
@janosh janosh deleted the from-dict-ret-self branch March 22, 2024 18:47
@DanielYang59
Copy link
Contributor

DanielYang59 commented Mar 23, 2024

Great work 😄 ! I can take care of the rest if you don't mind.

Meanwhile I noticed at least two modules are using naming conventions as with_something instead of from_something, do we have any plan to unify their namings as well?

image

@janosh
Copy link
Member Author

janosh commented Mar 23, 2024

good catch. i think we can rename those to from_... with a year-long deprecation period.

@DanielYang59
Copy link
Contributor

DanielYang59 commented Mar 23, 2024

Sure. I would do that (I saw your follow up in monty but that deprecation decorator change is still not merged.). With that deprecation would much cleaner and easier.

@janosh
Copy link
Member Author

janosh commented Mar 23, 2024

while we're at it, these classes should be renamed to PascalCase (e.g. BrunnerNNReciprocal) and also be deprecated. you can find them with this regex: class [^(= ]+_[^(]+\(

Screenshot 2024-03-23 at 06 42 45

@DanielYang59
Copy link
Contributor

Sure would do.

Can you please rebuild the docs (should be quite outdated) as we changed quite a lot in #3694? 🌹

@janosh
Copy link
Member Author

janosh commented Mar 23, 2024

invoke make-doc doesn't do a whole lot. not sure how doc updating is supposed to work atm. @shyuep?

Screenshot 2024-03-23 at 06 50 14

@DanielYang59
Copy link
Contributor

DanielYang59 commented Mar 23, 2024

Did you install sphinx dependencies (check warnings for invoke make-doc)? Like sphinx_markdown_builder and sphinx_rtd_theme? I got quite a lot of changes.

image

@janosh
Copy link
Member Author

janosh commented Mar 23, 2024

i did have those installed. i just updated sphinx_markdown_builder and sphinx_rtd_theme to v2 and ran again. that seems to have helped. still not many meaningful changes though. a few changed line ranges mostly af04258

@DanielYang59
Copy link
Contributor

Great to know. Meanwhile I noticed this warning from 3 years ago #2149:

warnings.warn(
"The substrate_analyzer module is being moved to the interfaces submodule in analysis."
" These imports will break in Pymatgen 2023",
category=FutureWarning,
stacklevel=2,
)

I assume it might be old enough to be removed?

DanielYang59 added a commit to DanielYang59/pymatgen that referenced this pull request Mar 29, 2024
janosh pushed a commit that referenced this pull request Mar 29, 2024
…rename `with_*` to `from_*` (#3725)

* rename `analysis` see end of #3702

* simply message

* rename classes to PascalCase

* rename classes to PascalCase

* move replacement close to old

* rename cls methods in `graphs`

* rename cls in `cp2k.sets`

* replace `with_empty_graph` with `from_empty_graph`

* replace `with_edges` with `from_edges`

* replace `with_local_env_strategy` with `from_local_env_strategy`

* rename `with_edges` to `from_edges`

* case fix

* DEBUG: test swapping decorator order

* swap `deprecated` and `classmethod` decorators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types Type all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants