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

Add examples, tests, enable Continuous Integration #6

Merged
merged 6 commits into from
Oct 12, 2022
Merged

Add examples, tests, enable Continuous Integration #6

merged 6 commits into from
Oct 12, 2022

Conversation

szabgab
Copy link
Contributor

@szabgab szabgab commented Oct 9, 2022

The failure on Windows seems to be a real bug.
If it is and if you don't have time to fix it I'd disable the CI on Windows till you find the time.

I've also written a blog post and recorded a video explaining this PR:
https://code-maven.com/python-automated-test-for-sssimp-simple-static-generator

Closes #5

@Tina-otoge Tina-otoge changed the base branch from master to setup-tests October 12, 2022 00:00
@Tina-otoge
Copy link
Owner

Thank you for looking into this so quickly!

I fiddled a bit locally and even watched the whole video. Thank you for using my project to showcase how to contribute to open source to others, this is a very great way to get more people into open source!!

Now regarding the PR,

  • I have a few concerns regarding the extensive use of the os.path module, while the code of sssimp itself mostly use Pathlib instead, which is more in-line with modern Python. I think it'll be better to use similar patterns in both the code and tests, rather than mixing 2 different ways of doing the same things.
  • I believe the test files and the conftest file should go to a /tests/ folder at the root of the project, this appears to be how it's done in most popular Python projects.
  • I'm not too sure about the --save design choice, I think it's not very intuitive to use pytest to generate the examples. Making it its own script might be better. Someone who is willing to re-generate the examples might not think to look into tests to see how they can easily generate them all.

Those are all very easy changes, so I'll merge this PR as-is into another branch and implement them myself, then merge it once it's ready.

Regarding the bugs you noticed, good catch!

I think the order of the files when you iterate over them should always be the same, even props to you for noticing you could fix it using Jinja, however I believe future users will expect being able to always get the file in the same order without having to use sort. Even if it might not be the order they want, it's weird for the tool to generate different outputs for seemingly random reasons.
Internally it calls pathlib.Path.glob, I guess this function does not produce predictable results!

for file in sssimp.CONTENT_DIR.glob('**/*.html'):

The Windows path one on .href is a good catch too. It happens because I'm simply returning the path of the underlying string representation of the path.Path object, which on Windows use backslashes.

I'll mark those 2 as bugs and fix them soon-ish.

Thank you again for your contribution, it is very appreciated!

@Tina-otoge Tina-otoge merged commit 4b57c9c into Tina-otoge:setup-tests Oct 12, 2022
@Tina-otoge Tina-otoge requested review from Tina-otoge and removed request for Tina-otoge October 12, 2022 00:29
@szabgab
Copy link
Contributor Author

szabgab commented Oct 12, 2022

I like your suggestions. As so far I avoided pathlib, this was a good opportunity to look into it.

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

Successfully merging this pull request may close these issues.

Add tests
2 participants