astropy / package-template

Template for packages that use Astropy. Maintainer: @astrofrog
http://docs.astropy.org/projects/package-template/en/latest/
Other
60 stars 63 forks source link

Conftest configure #483

Closed nstarman closed 3 years ago

nstarman commented 3 years ago
pllim commented 3 years ago

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 commented 3 years ago

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

embray commented 3 years ago

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 commented 3 years ago

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

nstarman commented 3 years ago

no-changelog-needed

nstarman commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.