Skip to content
This repository has been archived by the owner on Aug 11, 2023. It is now read-only.

Conftest configure #483

Merged
merged 1 commit into from
Nov 8, 2020
Merged

Conversation

nstarman
Copy link
Member

@nstarman nstarman commented Oct 4, 2020

  • the conftest code is given a doctoring from the top-level comments
  • the pytest_configure function is likewise documented
  • import astropy.units as u is auto-imported into docstring tests

@pllim
Copy link
Member

pllim commented Oct 5, 2020

auto-imported into docstring tests

I don't know how I feel about this. Explicit is better than implicit. People might forget this is there and then confusion ensues. 🤷

@nstarman
Copy link
Member Author

nstarman commented Oct 5, 2020

While I agree with Zen of Python line 2, I feel docstring examples are a somewhat special case. On the developer side, while writing example code in docstrings standard imports are often stuck in as boilerplate code at the beginning of the section or, if it's relevant, purposefully included with the specific example. While this doesn't (and shouldn't) change the latter, it nicely eliminates the former. Examples that don't want to show boilerplate are just the core code.
On the user side, this feels like a standard import that astropy users just know, like numpy -> np (which actually might be a good fixture to add).

Perhaps the numpy-to-np fixture is a better place to start and I submit the units-to-u as an issue? @pllim

@nstarman nstarman force-pushed the conftest_configure branch 4 times, most recently from ac66c09 to 3635edb Compare October 6, 2020 17:36
@embray
Copy link
Member

embray commented Oct 19, 2020

I agree with @pllim : It's cool to see that this is possible (I did not know this) but I think it's too magical, implicit, confusing (a new user stumbling on a particular doc does not necessarily know where np or u came from). It's also arbitrary bc I can think of several other "common imports" I would normally like to skip having to write when writing tests. At the end of the day, using implicit magic is also too prone to hide mistakes.

@nstarman
Copy link
Member Author

nstarman commented Oct 21, 2020

Okay. Outvoted 🙃. Not that this is a democracy 🗳. I'll drop the pytest auto-injections.

change comments to docstring
function docstring

Signed-off-by: Nathaniel Starkman <[email protected]>
@nstarman
Copy link
Member Author

#no-changelog-needed

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just out of curiosity what is the motivation for adding docstrings to the module and function? This file shouldn't appear in public API docs in general - pytest_configure for example is not something users should ever call directly, and the module should ideally be excluded from at API docs.

@nstarman
Copy link
Member Author

nstarman commented Oct 21, 2020

It's mostly personal reference. Even for short code snippets, with python being such a dynamical language, I like to know what I (or someone else) considered the scope of the function. It's also why I've taken to lightly type annotating my functions.

Also, my code linter showed a warning that a not-underscored function was missing a docstring. The linter and my personal preference match in this regard.

@embray
Copy link
Member

embray commented Oct 23, 2020

I don't see a problem with it--actually I think it makes sense. I think all modules should be documented with docstrings instead of comments, just for consistency's sake, even if it's not a docstring most people will ever see at runtime.

@embray embray merged commit c76dcc5 into astropy:cookiecutter Nov 8, 2020
@embray
Copy link
Member

embray commented Nov 8, 2020

This isn't doing anything more than adding the docstrings now, but I don't see the harm either. Thanks @nstarman ! I'll try to follow up on some of your other PRs sooner rather than later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants