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

Clean up CONTRIBUTING.md docs and change _is_named_isotope getattr default value #4174

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Nov 15, 2024

  • [I believe this would not be breaking] The default value as False over None might be more conventional (we should not use truthy to check None):
    # When the `_is_named_isotope` doesn't have `_is_named_isotope`,
    # we're effectively checking `if None`
    - if getattr(obj, "_is_named_isotope", None):
    + if getattr(obj, "_is_named_isotope", False):
  • Recommend Google Style Docstring in CONTRIBUTING.md
  • Clarify usage of PMG_TEST_FILES_DIR when running unit test
  • Fix internal indexes


```sh
git clone https://github.com/<username>/pymatgen

# (Alternative/Much Faster) If you don't need a full commit history/other branches
# git clone --depth 1 https://github.com/<username>/pymatgen
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be good to provide this alternative as I think most new contributors don't really need the complete commit history or other branches, they would just branch from main.

To clone the complete repo:

git clone https://github.com/materialsproject/pymatgen.git  48.11s user 15.68s system 69% cpu 1:31.62 total

Depth one clone:

git clone https://github.com/materialsproject/pymatgen.git --depth 1  7.60s user 2.75s system 31% cpu 32.622 total

@DanielYang59 DanielYang59 marked this pull request as ready for review November 15, 2024 06:13
@DanielYang59 DanielYang59 marked this pull request as draft November 15, 2024 10:21
@DanielYang59 DanielYang59 marked this pull request as ready for review November 15, 2024 10:23

```sh
cd path/to/repo
pip install -e . # install the package in your environment as "editable" == dev package
PMG_TEST_FILES_DIR=$(pwd)/tests/files pytest tests # run the test suite providing the path for the datafiles
Copy link
Contributor Author

@DanielYang59 DanielYang59 Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe setting PMG_TEST_FILES_DIR is not needed for editable mode, however we might still leave the "non-editable install + set PMG_TEST_FILES_DIR" as an option here, as there might be users who want to install the package in non-editable mode but still run tests, say people who don't intend to develop pymatgen and just run tests to verify pymatgen integration.

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

Successfully merging this pull request may close these issues.

1 participant