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

Implement src folder #15

Closed
ryancheley opened this issue Nov 29, 2021 · 15 comments
Closed

Implement src folder #15

ryancheley opened this issue Nov 29, 2021 · 15 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@ryancheley
Copy link
Owner

Is your feature request related to a problem? Please describe.

As reported in issue #14 the conversion to using a questions.json file broke the package.

Describe the solution you'd like
As recommended by @matthewfeickert using a src folder will help with this, details seen here PyPA Packaging Python Projects tutorial

Describe alternatives you've considered
None

Additional context
In this particular case the implementation of a custom API endpoint on the web somewhere to serve up the questions via json would also work, but seems like a bit of overkill

@ryancheley
Copy link
Owner Author

I was able to refactor to get the package moved to a src folder. The bit that tripped me up was how to call it from the new folder.

It turns out all that needed to be added was this line:

package_dir = {‘’: ‘src’}

Still working out how to get the questions.json file to come with the package

@ryancheley
Copy link
Owner Author

Hints on how to reference the questions.json file here

@ryancheley
Copy link
Owner Author

was able to read the questions.json file with use of importlib.resources with these lines:

with importlib.resources.open_text("the_well_maintained_test.data", "questions.json") as file:
    questions = json.load(file)          

@ryancheley
Copy link
Owner Author

The package works now, but the tests won’t run. Ugh

@ryancheley
Copy link
Owner Author

ryancheley commented Nov 30, 2021

This is what is output:

ERROR collecting src/tests/test_the_well_maintained_test.py ______________
ImportError while importing test module '/Users/ryan/Documents/github/the-well-maintained-test/src/tests/test_the_well_maintained_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
src/tests/test_the_well_maintained_test.py:40: in <module>
    from the_well_maintained_test.cli import cli
src/the_well_maintained_test/cli.py:8: in <module>
    from rich.console import Console
E   ModuleNotFoundError: No module named 'rich'

however this shows that rich is installed

❯ pip list | grep rich
rich                     10.15.1

@matthewfeickert
Copy link
Contributor

The package works now, but the tests won’t run. Ugh

Can you make a PR and I can look at it (probably much later tonight or tomorrow)? Or push your dev branch up so that I can check it out on a fork?

@ryancheley
Copy link
Owner Author

@matthewfeickert just pushed up branch for v0.6.0

I'm going to try to keep banging my head against the wall for a while to see if I can figure it out too ☺️

@ryancheley
Copy link
Owner Author

And of course within 5 minutes of pushing up my code (and taking a few hours away) I found the issue.

While trying to get everything to work earlier today I removed my virtual environment and then recreated it. When I did that I didn't seem to install all of the needed packages, for example, coverage. And since my justfile has a recipe which calls coverage I got an error (not the one I would have expected though!)

Installing coverage resolved the issue.

@ryancheley
Copy link
Owner Author

And of course within 5 minutes of pushing up my code (and taking a few hours away) I found the issue.

While trying to get everything to work earlier today I removed my virtual environment and then recreated it. When I did that I didn't seem to install all of the needed packages, for example, coverage. And since my justfile has a recipe which calls coverage I got an error (not the one I would have expected though!)

Installing coverage resolved the issue.

All that being said, @matthewfeickert if you'd be willing to take a look at the changes I made and offer any feedback I'd be grateful ☺️

@matthewfeickert
Copy link
Contributor

matthewfeickert commented Dec 1, 2021

All that being said, @matthewfeickert if you'd be willing to take a look at the changes I made and offer any feedback I'd be grateful relaxed

Sure! 👍 Just from doing a visual spot check between main...v0.6.0 things look good but I'd probably want to also run packaging tests with build and then check to make sure everything is getting in there correctly (aka, checking the contents of the sdist and the wheel with python -m tarfile --list dist/*tar.gz and python -m zipfile --list dist/*.whl). I am currently writing a talk for tomorrow, but I'll try to take a quick look over tonight before I sign off or tomorrow morning.

@ryancheley ryancheley added this to the V0.6.0 milestone Dec 1, 2021
@matthewfeickert
Copy link
Contributor

Sorry for the deloadyThe JSON file is getting included in the sdist and wheel but it isn't being found properly after install (this is cloning down the repo and checking out the v0.6.0 branch)

(venv) root@2114cca7921b:/the-well-maintained-test# git branch
  main
* v0.6.0
(venv) root@2114cca7921b:/the-well-maintained-test# git diff origin/v0.6.0
(venv) root@2114cca7921b:/the-well-maintained-test# python -m build .
(venv) root@2114cca7921b:/the-well-maintained-test# python -m tarfile --list dist/*.tar.gz
the-well-maintained-test-0.6.0/ 
the-well-maintained-test-0.6.0/LICENSE 
the-well-maintained-test-0.6.0/PKG-INFO 
the-well-maintained-test-0.6.0/README.md 
the-well-maintained-test-0.6.0/pyproject.toml 
the-well-maintained-test-0.6.0/setup.cfg 
the-well-maintained-test-0.6.0/setup.py 
the-well-maintained-test-0.6.0/src/ 
the-well-maintained-test-0.6.0/src/the_well_maintained_test/ 
the-well-maintained-test-0.6.0/src/the_well_maintained_test/__init__.py 
the-well-maintained-test-0.6.0/src/the_well_maintained_test/cli.py 
the-well-maintained-test-0.6.0/src/the_well_maintained_test/data/ 
the-well-maintained-test-0.6.0/src/the_well_maintained_test/data/questions.json 
the-well-maintained-test-0.6.0/src/the_well_maintained_test/helpers.py 
the-well-maintained-test-0.6.0/src/the_well_maintained_test/utils.py 
the-well-maintained-test-0.6.0/src/the_well_maintained_test.egg-info/ 
the-well-maintained-test-0.6.0/src/the_well_maintained_test.egg-info/PKG-INFO 
the-well-maintained-test-0.6.0/src/the_well_maintained_test.egg-info/SOURCES.txt 
the-well-maintained-test-0.6.0/src/the_well_maintained_test.egg-info/dependency_links.txt 
the-well-maintained-test-0.6.0/src/the_well_maintained_test.egg-info/entry_points.txt 
the-well-maintained-test-0.6.0/src/the_well_maintained_test.egg-info/requires.txt 
the-well-maintained-test-0.6.0/src/the_well_maintained_test.egg-info/top_level.txt 
(venv) root@2114cca7921b:/the-well-maintained-test# python -m zipfile --list dist/*.whl
File Name                                             Modified             Size
the_well_maintained_test/__init__.py           2021-12-01 20:46:28            0
the_well_maintained_test/cli.py                2021-12-01 20:46:28         6338
the_well_maintained_test/helpers.py            2021-12-01 20:46:28         1921
the_well_maintained_test/utils.py              2021-12-01 20:46:28         8562
the_well_maintained_test/data/questions.json   2021-12-01 20:46:28          667
the_well_maintained_test-0.6.0.dist-info/LICENSE 2021-12-01 20:46:44        11357
the_well_maintained_test-0.6.0.dist-info/METADATA 2021-12-01 20:46:44         4844
the_well_maintained_test-0.6.0.dist-info/WHEEL 2021-12-01 20:46:44           92
the_well_maintained_test-0.6.0.dist-info/entry_points.txt 2021-12-01 20:46:44           97
the_well_maintained_test-0.6.0.dist-info/top_level.txt 2021-12-01 20:46:44           25
the_well_maintained_test-0.6.0.dist-info/RECORD 2021-12-01 20:46:44         1044
(venv) root@2114cca7921b:/the-well-maintained-test# python -m pip install dist/*.whl
Processing ./dist/the_well_maintained_test-0.6.0-py3-none-any.whl
Collecting click
  Downloading click-8.0.3-py3-none-any.whl (97 kB)
     |████████████████████████████████| 97 kB 2.8 MB/s             
Requirement already satisfied: requests in /venv/lib/python3.9/site-packages (from the-well-maintained-test==0.6.0) (2.26.0)
Collecting rich
  Downloading rich-10.15.1-py3-none-any.whl (214 kB)
     |████████████████████████████████| 214 kB 17.4 MB/s            
Requirement already satisfied: idna<4,>=2.5 in /venv/lib/python3.9/site-packages (from requests->the-well-maintained-test==0.6.0) (3.3)
Requirement already satisfied: certifi>=2017.4.17 in /venv/lib/python3.9/site-packages (from requests->the-well-maintained-test==0.6.0) (2021.10.8)
Requirement already satisfied: charset-normalizer~=2.0.0 in /venv/lib/python3.9/site-packages (from requests->the-well-maintained-test==0.6.0) (2.0.8)
Requirement already satisfied: urllib3<1.27,>=1.21.1 in /venv/lib/python3.9/site-packages (from requests->the-well-maintained-test==0.6.0) (1.26.7)
Requirement already satisfied: pygments<3.0.0,>=2.6.0 in /venv/lib/python3.9/site-packages (from rich->the-well-maintained-test==0.6.0) (2.10.0)
Requirement already satisfied: colorama<0.5.0,>=0.4.0 in /venv/lib/python3.9/site-packages (from rich->the-well-maintained-test==0.6.0) (0.4.4)
Collecting commonmark<0.10.0,>=0.9.0
  Downloading commonmark-0.9.1-py2.py3-none-any.whl (51 kB)
     |████████████████████████████████| 51 kB 11.1 MB/s            
Installing collected packages: commonmark, rich, click, the-well-maintained-test
Successfully installed click-8.0.3 commonmark-0.9.1 rich-10.15.1 the-well-maintained-test-0.6.0
(venv) root@2114cca7921b:/the-well-maintained-test# python -m pip show the-well-maintained-test
Name: the-well-maintained-test
Version: 0.6.0
Summary: Programatically tries to answer the 12 questions from         Adam Johnson's blog post https://adamj.eu/tech/2021/11/04/the-well-maintained-test/
Home-page: https://github.com/ryancheley/the-well-maintained-test
Author: Ryan Cheley
Author-email: 
License: Apache License, Version 2.0
Location: /venv/lib/python3.9/site-packages
Requires: click, requests, rich
Required-by: 
(venv) root@2114cca7921b:/the-well-maintained-test# find /venv/lib/python3.9/site-packages -iname "questions.json"
/venv/lib/python3.9/site-packages/the_well_maintained_test/data/questions.json
(venv) root@2114cca7921b:/the-well-maintained-test# cd
(venv) root@2114cca7921b:~# the-well-maintained-test questions
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/importlib/resources.py", line 97, in open_binary
    return open(full_path, mode='rb')
FileNotFoundError: [Errno 2] No such file or directory: '/root/questions.json'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/venv/bin/the-well-maintained-test", line 8, in <module>
    sys.exit(cli())
  File "/venv/lib/python3.9/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/venv/lib/python3.9/site-packages/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/venv/lib/python3.9/site-packages/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/venv/lib/python3.9/site-packages/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/venv/lib/python3.9/site-packages/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/venv/lib/python3.9/site-packages/the_well_maintained_test/cli.py", line 168, in questions
    with importlib.resources.open_text("the_well_maintained_test.data", "questions.json") as file:
  File "/usr/local/lib/python3.9/importlib/resources.py", line 121, in open_text
    open_binary(package, resource), encoding=encoding, errors=errors)
  File "/usr/local/lib/python3.9/importlib/resources.py", line 111, in open_binary
    raise FileNotFoundError(message)
FileNotFoundError: 'questions.json' resource not found in 'the_well_maintained_test.data'
(venv) root@2114cca7921b:~# 

@matthewfeickert
Copy link
Contributor

matthewfeickert commented Dec 1, 2021

Oh I see also that you have tests under src.

(venv) root@2114cca7921b:/the-well-maintained-test# git ls-files -- src/
src/tests/__init__.py
src/tests/test_classes.py
src/tests/test_the_well_maintained_test.py
src/the_well_maintained_test/__init__.py
src/the_well_maintained_test/cli.py
src/the_well_maintained_test/data/__init__.py
src/the_well_maintained_test/data/questions.json
src/the_well_maintained_test/helpers.py
src/the_well_maintained_test/utils.py

You don't want that. Part of the motivation for using src is to ensure that your tests only see an installed version (let me go find the original blog posts for this motivation).

edit:

From scikit-hep/pyhf#513:

Instead of trying to summarize here why I think this should be the case I will let others do so for me:

TLDR: Use a src directory structure to ensure that the code that is tested is the code that is shipped in the distribution and nothing else.

If you just read one thing read Hynek's blog post — it, like nearly all Hynek's writing, is quite good. Though that blog post also recommends reading Ionel's blog post. :)

@matthewfeickert
Copy link
Contributor

matthewfeickert commented Dec 1, 2021

At the moment you're using importlib.resources

(venv) root@2114cca7921b:/the-well-maintained-test# git grep "questions.json" | head -n 1
src/the_well_maintained_test/cli.py:    with importlib.resources.open_text("the_well_maintained_test.data", "questions.json") as file:

which doesn't work. You can usepkg_resources though which will

>>> import pkg_resources
>>> help(pkg_resources.resource_filename)
Help on method resource_filename in module pkg_resources:

resource_filename(package_or_requirement, resource_name) method of pkg_resources.ResourceManager instance
    Return a true filesystem path for specified resource

>>> from pathlib import Path
>>> questions_path = Path(
...     pkg_resources.resource_filename(
...         "the_well_maintained_test", str(Path("data").joinpath("questions.json"))
...     )
... )
>>> questions_path.exists()
True
>>> with open(questions_path) as read_file:
...     read_file.read()
... 
'{\n    "1": "1. Is it described as “production ready”?",\n    "2": "2. Is there sufficient documentation?",\n    "3": "3. Is there a changelog?",\n    "4": "4. Is someone responding to bug reports?",\n    "5": "5. Are there sufficient tests?",\n    "6": "6. Are the tests running with the latest <Language> version?",\n    "7": "7. Are the tests running with the latest <Integration> version?",\n    "8": "8. Is there a Continuous Integration (CI) configuration?",\n    "9": "9. Is the CI passing?",\n    "10": "10. Does it seem relatively well used?",\n    "11": "11. Has there been a commit in the last year?",\n    "12": "12. Has there been a release in the last year?"\n}\n'

Though with src/the_well_maintained_test/cli.py you can get away with using __name__

(venv) root@2114cca7921b:/the-well-maintained-test# git diff
diff --git a/src/the_well_maintained_test/cli.py b/src/the_well_maintained_test/cli.py
index 2dbc11b..a13b288 100644
--- a/src/the_well_maintained_test/cli.py
+++ b/src/the_well_maintained_test/cli.py
@@ -1,9 +1,9 @@
-import importlib.resources
 import json
-import pathlib
+from pathlib import Path
 from urllib.parse import urlparse
 
 import click
+import pkg_resources
 import requests
 from rich.console import Console
 from rich.padding import Padding
@@ -64,7 +64,7 @@ def auth(auth: str) -> None:  # pragma: no cover
     "Save authentication credentials to a JSON file"
     console.print("Create a GitHub personal user token and paste it here:")
     personal_token = Prompt.ask("Personal token")
-    if pathlib.Path(auth).exists():
+    if Path(auth).exists():
         auth_data = json.load(open(auth))
     else:
         auth_data = {}
@@ -96,7 +96,7 @@ def url(url: str, branch: str, progress: bool) -> None:  # pragma: no cover
     if url[-1] == "/":
         url = url.strip("/")
 
-    with importlib.resources.open_text("the_well_maintained_test.data", "questions.json") as file:
+    with open(Path(pkg_resources.resource_filename(__name__, str(Path("data").joinpath("questions.json"))))) as file:
         questions = json.load(file)
 
     parse_object = urlparse(url)
@@ -165,7 +165,7 @@ def url(url: str, branch: str, progress: bool) -> None:  # pragma: no cover
 )
 def questions(question: str) -> None:  # pragma: no cover
     "List of questions tested"
-    with importlib.resources.open_text("the_well_maintained_test.data", "questions.json") as file:
+    with open(Path(pkg_resources.resource_filename(__name__, str(Path("data").joinpath("questions.json"))))) as file:
         questions = json.load(file)
 
     if question != "all":
(venv) root@2114cca7921b:/the-well-maintained-test# rm -rf dist && python -m build . && python -m pip install --force-reinstall dist/*.whl && the-well-maintained-test questions
...
Successfully installed certifi-2021.10.8 charset-normalizer-2.0.8 click-8.0.3 colorama-0.4.4 commonmark-0.9.1 idna-3.3 pygments-2.10.0 requests-2.26.0 rich-10.15.1 the-well-maintained-test-0.6.0 urllib3-1.26.7
1. Is it described as “production ready”?
2. Is there sufficient documentation?
3. Is there a changelog?
4. Is someone responding to bug reports?
5. Are there sufficient tests?
6. Are the tests running with the latest <Language> version?
7. Are the tests running with the latest <Integration> version?
8. Is there a Continuous Integration (CI) configuration?
9. Is the CI passing?
10. Does it seem relatively well used?
11. Has there been a commit in the last year?
12. Has there been a release in the last year?

@ryancheley
Copy link
Owner Author

Closed with fc6b584

@ryancheley
Copy link
Owner Author

@matthewfeickert thanks for all of your help on this! It was greatly appreciated 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants