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 docstrings and refs #256

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Clean docstrings and refs #256

merged 3 commits into from
Sep 25, 2023

Conversation

brynpickering
Copy link
Contributor

A few things changed here:

  1. A temporary fix for Abstractmethod docstrings are not inherited in API reference #255 (which should stay open after merging this PR since we are ultimately waiting for an upstream fix) by reintroduing duplicate docstrings in child classes of DiscretionaryTrip.
  2. Removed all un-referenced graphics. They can always be resurected from old commits if a user wants to reference them again.
  3. Better cross-referencing in the docs, particlarly to the PAM API docs (e.g., [][pam.core.Person] will create a link to https://arup-group.github.io/pam/develop/reference/pam/core/#pam.core.Person).
  4. Add member inheritance, so that child classes show their parent class methods in the API docs too. It adds duplication to the API docs but does make it easier to view what methods are available for a class without needing to follow the inheritance chain.

Checklist

Any checks which are not relevant to the PR can be pre-checked by the PR creator.
All others should be checked by the reviewer(s).
You can add extra checklist items here if required by the PR.

  • CHANGELOG updated
  • Tests added to cover contribution
  • Documentation updated

@codecov-commenter
Copy link

Codecov Report

Merging #256 (10617b8) into main (07813c8) will increase coverage by 0.78%.
Report is 131 commits behind head on main.
The diff coverage is 87.53%.

@@            Coverage Diff             @@
##             main     #256      +/-   ##
==========================================
+ Coverage   86.84%   87.62%   +0.78%     
==========================================
  Files          49       48       -1     
  Lines        5496     5739     +243     
  Branches     1372     1438      +66     
==========================================
+ Hits         4773     5029     +256     
+ Misses        462      440      -22     
- Partials      261      270       +9     
Files Changed Coverage Δ
pam/planner/od.py 97.33% <ø> (-0.04%) ⬇️
pam/read/__init__.py 100.00% <ø> (ø)
pam/write/__init__.py 100.00% <ø> (ø)
pam/activity.py 90.90% <50.00%> (+0.14%) ⬆️
pam/write/matsim.py 76.08% <59.25%> (-2.58%) ⬇️
pam/optimise/grid.py 76.74% <70.00%> (+33.88%) ⬆️
pam/optimise/random.py 89.09% <71.42%> (+47.91%) ⬆️
pam/planner/utils_planner.py 81.70% <76.59%> (-2.51%) ⬇️
pam/planner/choice_location.py 88.39% <80.85%> (-5.55%) ⬇️
pam/plot/plans.py 80.53% <81.81%> (+2.29%) ⬆️
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Theodore-Chatziioannou Theodore-Chatziioannou left a comment

Choose a reason for hiding this comment

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

looks good, but I would rather revert the set change to pd.unique before merging, as per my comment above.

pam/planner/od.py Show resolved Hide resolved
pam/planner/od.py Show resolved Hide resolved
@@ -446,8 +439,7 @@ def leg_ratio_p(self) -> np.array:
@property
def diversion_factors(self) -> np.array:
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you re-introducing long lines here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docstrings wrap in tooltips etc. anyway, so it is better to write them more "naturally" and let the downstream tools to decide where the linebreaks are.

I tend to opt to set linebreaks at the end of sentences. This also helps to ensure you're not accidentally writing excessively long sentences (as you can visually parse the length of your sentences quickly). This is good because long sentences will lead to poor information transfer!

There's obviously a compromise with legibility in the code itself (lines that are too long might be difficult to parse). This is partially solved by keeping sentences short. But also most IDEs allow line wrapping which will automatically deal with long docstring lines.

@brynpickering brynpickering merged commit 8631079 into main Sep 25, 2023
13 of 14 checks passed
@brynpickering brynpickering deleted the clean-docstrings-and-refs branch September 25, 2023 09:47
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.

3 participants