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

Documentation improvements (update import statements, include scripts) #1610

Merged
merged 28 commits into from
Aug 5, 2024

Conversation

VeckoTheGecko
Copy link
Contributor

@VeckoTheGecko VeckoTheGecko commented Jul 23, 2024

This PR implements the following changes:

Blocked by #1608

VeckoTheGecko and others added 23 commits July 22, 2024 14:53
Pre-commit doesn't support prettier anymore. Moving to biome
Makes all pages go from `/pagename.html` to `/pagename`. RTD will redirect these properly, so old links aren't broken
https://docs.readthedocs.io/en/stable/user-defined-redirects.html#clean-html-urls-redirects
Based on the patch proposed by @VeckoTheGecko
…_JIT

Support use of parcels.rng in Kernels
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
docs/documentation/additional_examples.rst Outdated Show resolved Hide resolved
docs/documentation/index.rst Show resolved Hide resolved
parcels/kernel.py Outdated Show resolved Hide resolved
VeckoTheGecko added a commit that referenced this pull request Jul 30, 2024
VeckoTheGecko added a commit that referenced this pull request Jul 30, 2024
@VeckoTheGecko
Copy link
Contributor Author

VeckoTheGecko commented Jul 30, 2024

I guess force pushing while referencing a commit glitches out the UI a bit. This is just a single commit in the git log

@VeckoTheGecko
Copy link
Contributor Author

Thought I'd wrap #1636 into here

Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

In principle OK, but I don't entire understand why we still use from datetime import timedelta.

Given also the other changes, would it not be more consistent to do import datetime and then use datetime.timedelta throughout?

@VeckoTheGecko
Copy link
Contributor Author

VeckoTheGecko commented Aug 5, 2024

The changes for the import statements are related to how these imports are typically done by the Python community, and reducing confusion for software developers. The imports for os, and ast resulted in functions/modules like parse() and path being imported into the global namespace which can lead to confusion as path as a variable is mainly used as to represent a filepath object/string, and parse could be thought of as a user made function. Writing ast. or os. is not much more of a burden to type out, and signal clearly where the function is coming from.

Similarly, changing delta to timedelta also reduces confusion. from datetime import timedelta is a common import, so seeing timedelta by itself already signals its from datetime. We could do import datetime and datetime.timedelta, but its a bit of a pain to type out especially since this method is used a lot in parcels. We could shorten that by doing import datetime as dt (which is another common alias), but dt is used quite a bit in our function calls so not optimal. I thought timedelta was best.

@VeckoTheGecko VeckoTheGecko merged commit 09d8369 into master Aug 5, 2024
10 checks passed
@VeckoTheGecko VeckoTheGecko deleted the v/update-notebooks branch August 5, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Removing delta alias for timedelta Changing Parcels -> parcels and importing all of parcels throughout
2 participants