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

Pybuilder introduction #4

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Pybuilder introduction #4

wants to merge 10 commits into from

Conversation

svaningelgem
Copy link

Hello,

This feature is to introduce a bit more structured way of building this project.
However, this is not 100% correct yet as a few of the tests are failing still.

But I'd love to have your initial opinion on this already.

BTW, I moved a lot of files, but no substantial changes (besides the automated "Optimize Imports" from IntelliJ).
The main gist of the changes is in build.py

Grtz,
Steven

@tools4origins
Copy link
Owner

Hello Steven and thank you for this PR!

Some changes were introduced in pysparkling last week, in particular the project has now been moved to the pysparkling organization. This is why I didn't comment your PR yet.

Could you do your PR there, rebasing on its master branch? I am currently merging all the differences from this repo's master to the pysparkling one (should be done by end of week).

Also, can we not move the code to a src/main/python directory? This standard is mostly jvm-specific whereas pysparkling is already using the more frequently used in python projects convention of putting the library code in a folder named based on a library.

Last, could you not optimize imports in IntelliJ? It creates a lot of diff which are not that useful, and we could address adopting a stricter code style convention if needed in another PR. There's a tick box in the commit dialog to disable optimization from what I remember.

@svaningelgem
Copy link
Author

ok for the re-doing it there.

Disable optimize & do another PR to clean up the order of imports: ok

Rationale for the dir structure:
I do like the source tree structure a lot though because it keeps the code nicely organised. Right now if you want to test something, you need to set your PYTHONPATH to the root directory. Which means that all other files and directories are potential imports as well. Not so clean imho.
Also, I like the tests separated from the code tree (and if you would put them inside a tests directory, they're a potential import as well).

I do know that the dir structure is lent/stolen/borrowed from the java world, but that doesn't mean it's bad, isn't it? I'm all for clean code, but potentially allowing non-related imports is not clean.
BTW, the link is from 2013. 1 year earlier PEP 420 came out, so I'm not certain that the author took this into account for his rationale? Py3.3 was just out, so maybe yes, or maybe not? I can't really figure that out from a cursory read.
I think not, because he wrote "Any directory with an init.py file is considered a Python package.". Which is not entirely true after 3.3.

Anyway, I know what is, but that doesn't mean it has to stay that way ;-). What would you propose?
Maybe something like:

docs/
scripts/
src/pysparkling/
tests/

That way the modules are all nicely hidden away under src, with no potential imports from for example the scripts directory.

@tools4origins
Copy link
Owner

Hi,

Thanks for providing the rationale, you convince me of the limitation of the current structure and I like the latest one you proposed.
I think it is the one recommended by pypa and it appears to be adopted by some projects like flake8 and black.

Let's go for it!

@svaningelgem
Copy link
Author

;-) Happy to hear that I am right for a change 😛 . Just let me know when I can do the changes on the other source tree. (because you mentioned: I am currently merging all the differences from this repo's master to the pysparkling one (should be done by end of week).). I just don't want to have too much merge conflicts.

@tools4origins
Copy link
Owner

The differences are now merged, with a heavy rewrite of the git history, you may want to rebase your commit from this branch to the new repo branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants