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

Questions regarding Python examples #45

Open
caichinger opened this issue Dec 7, 2023 · 5 comments
Open

Questions regarding Python examples #45

caichinger opened this issue Dec 7, 2023 · 5 comments

Comments

@caichinger
Copy link
Contributor

I really like this kata - thank you for all the effort that went into it and sharing it publicly!

I have a few questions regarding the Python examples.

What is the intended difference between python/ and python_pytest/?

To me, the distinction is not clear.

For me, python_pytest/ suggest that this version uses pytest whereas the other uses unittest.
However, both examples use pytest.

The code base is almost identical in both cases, except for the difference in the README and the additional textest_fixture.py in the python_pytest/ version.

Data Files for texttest_fixture.py missing?

texttest_fixture.py loads catalog.csv (and other files).

Is a user intended to create these file by themselves or were they maybe forgotten?

How to run tests in python_pytest/?

With the current layout/configuration, the tests in python_pytest/ cannot be run by simply invoking $ pytest, since the required modules cannot be imported.

At the moment, the README suggests use pytest to run tests but it may no be immediately clear how to do that.

There are different ways to address this issue:

  1. Change layout by moving tests/ into src/ (mimicking the layout currently used in python/)
  2. Add a pytest.ini to python_pytest/ where we add src/ to the pythonpath.
  3. Add a conftest.py to python_pytest/

If you share your intention with me, I am happy to provide a PR.

Personally, I would kind of merge the two folders.

@codecop
Copy link
Contributor

codecop commented Dec 20, 2023

Thank you Claus. Did you also check the with_tests branch and compare both versions?

@caichinger
Copy link
Contributor Author

caichinger commented Dec 20, 2023

Thank you Claus. Did you also check the with_tests branch and compare both versions?

Good point. Thank you for bringing that up. No, I did not check.
I just checked and my questions remain.

Re: Data Files for texttest_fixture.py missing?

The required files exist in the with_texts branch (and not in main).

@codecop
Copy link
Contributor

codecop commented Dec 21, 2023

Yes the idea of two branches is usually this:

  • main branch has all the code and a simple, failing unit test. Sometimes proper setup with different frameworks, which has been the case here as it seems. Usually this branch has no reference to approvals. So e.g. the file approvaltests_config.json should be deleted in both folders.
  • with_tests has the same code, same setup, all the same PLUS complete test coverage, often using approvals. This looks good.
  • I see now. texttest_fixture is often a file also present as a preparation for using texttest, an external tool. It is just a main running the code. We also have it in GildedRose like that. its data files should not exist on main branch.
  • To clarify, on main the coder is supposed to complete texttest running, if that is the task (usually it is not).

I agree both versions could be combined in a reasonable way in python using the layout of pytest with added config files which are missing. Needs 2 PR one for main and one for with_tests. Happy to receive a PR from you Claus

@caichinger
Copy link
Contributor Author

Happy to receive a PR from you Claus

Thank you the context. I will prepare a PR based on how I understand what I see and what you said and then we continue from there.

@caichinger
Copy link
Contributor Author

Writing to let you know that I did not forget about this issues and that I do not have time for it now but I will get back to it in February.

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

No branches or pull requests

2 participants