Update deps and make pooch a test+docs dependency only #158
Conversation
| @@ -29,6 +29,7 @@ | |||
| "import plopp as pp\n", | |||
| "from ess import sans\n", | |||
| "from ess import isissans as isis\n", | |||
| "import ess.isissans.data # noqa: F401\n", | |||
There was a problem hiding this comment.
Using noqa directives in the docs is not great. Can you refactor the notebook to use the imported module directly?
There was a problem hiding this comment.
This has already made it into other packages such as ESSdiffraction. Is it really worth the effort?
There was a problem hiding this comment.
The packages don't all have to be equally bad. I think it is important to keep the docs simple given our target audience. The comments are distracting. In particular because they deal with a concept (static analysers) that many users won't even know exists.
There was a problem hiding this comment.
I suppose we have different opinions on this: Having to use a module imported under an additional different name is likewise increasing the cognitive overhead. I think most users never even look at the cell doing module imports, they are more likely to look at the code further down.
If you feel really strongly about this, please push an update.
There was a problem hiding this comment.
I can push an update, I also think it's cleaner without the noqa ;-)
There was a problem hiding this comment.
Update: having gone through the different notebooks and started making the changes, I am changing my mind on this. I now agree that it's more confusing to have two different modules to get things from, and only remembering one is easier.
In addition, this may all change again if we make a separate common essdata package, so minimizing the number of changes here by adding the noqa import line is fine I think.
No description provided.