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

Use application registry in docs #1275

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

Conversation

cpascual
Copy link
Contributor

@cpascual cpascual commented Mar 24, 2021

The application registry feature is not adequately advertised.
Improve this by using get_application_registry() instead of UnitRegistry()
throughout the docs (including README and docstrings) when that makes sense.
Also change the tutorial to recommend using it whenever possible.

See #1273 for the motivation.

  • Closes # (insert issue number)
  • Executed pre-commit run --all-files with no errors
  • The change is fully covered by automated unit tests
  • Documented in docs/ as appropriate
  • Added an entry to the CHANGES file

Carlos Pascual added 4 commits March 24, 2021 12:02
The application registry feature is not adequately advertised.
Improve this by using get_application_registry() instead of UnitRegistry()
throughout the docs (including README and docstrings) when that makes sense.
Also change the tutorial to recommend using it whenever possible.

See hgrecco#1273 for the motivation.
@cpascual
Copy link
Contributor Author

cpascual commented Mar 24, 2021

Hi, I am still getting some doctest errors, but I am not sure if they are related to my changes.

@hgrecco
Copy link
Owner

hgrecco commented Mar 24, 2021

I am not sure that putting ureg.get_application_registry everywhere is a good idea but I might biased by my own history with pint.

What I already know is that we should improve the correspondending subsection (or even move it to its own section). This should include the rationale that I gave in #1273 and maybe a small example (e.g. project 1, project 2 ..). If we put a new section, we could also link to wraps aand check and other things that allow pint to play well with others.

Going back to the readme and docs, my fear is that we are changing the suggested way to use it too quickly and confuse existing users. My suggestions would be to do a gradual rollout. Example: we put the get_application_registry in the readme and cos as a note after we show how to instantiate the registry and I see how it plays off. But, as I said, I might be dead wrong so I would be happy to hear others peoples thoughts.

@cpascual
Copy link
Contributor Author

I completely defer it to your judgement, @hgrecco since I have only a very biased perspective on the use of pint.

I created this PR just because it was simpler than trying to explain my proposed changes, but I don't mind at all if it is partially or completely edited.

My point of view: I like the "radical" change (as proposed) because the full benefit of the application registry comes, IMHO, when its use is generalized (i.e. when most pint-enabled projects play along well with the rest)

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.

2 participants