harelba / q

q - Run SQL directly on delimited files and multi-file sqlite databases
http://harelba.github.io/q/
GNU General Public License v3.0
10.19k stars 421 forks source link

BUG: fix broken packaging #327

Open Sann5 opened 9 months ago

Sann5 commented 9 months ago

Closes #326 See issue above for detailed explanation.

FriedrichFroebel commented 9 months ago

Even with this patch, this does not look correct as this will install a package named bin when one wants to install the package q. The usual approach for packaging single-file distributions usually is to have the corresponding file inside the top-level directory (which will install the file into the site-packages without a wrapping directory) or to use q/__init__.py (and maybe q/__main__.py for CLI-related stuff) instead.

Sann5 commented 9 months ago

I agree. I mentioned this in the issue linked in the description (additional considerations section). But since it involves a more serious refactoring I was hoping to get the opinion from one of the maintainers before moving forward.

Sann5 commented 8 months ago

@FriedrichFroebel what would you like me to name the package? I also move the tests into the package so that they can be run by a CI (need this to make q available in conda. See this issue: #322 )

FriedrichFroebel commented 8 months ago

what would you like me to name the package?

Usually q.

I also move the tests into the package so that they can be run by a CI

This should not be required if you do proper packaging of source and binary distributions. The tests directory can stay at the root level of the repository and should be packaged inside the sdist by default, while binary distributions will only contain the actual q code without the tests.

Sann5 commented 8 months ago

Putting the tests inside the src directory makes it easier to run the tests in the CI for conda forge. I can try to leave them as is but if it's not a big deal to you I would suggest leaving them inside. I think tests are nice when making a tool like this available (for example I discovered that q can break with Python >= 3.11). What's more, there is no binary for Linux which is my main motivation for making q available in conda.

Sann5 commented 8 months ago

This is the layout I propose based on what I have seen on best practices, naming conventions, and ease of testing @FriedrichFroebel.

...
├── setup.py
├── src
│   ├── q
│   │   ├── __init__.py
│   │   ├── q.bat
│   │   ├── q.py
│   │   └── tests
│   │       ├── BENCHMARK.md
│   │       ├── __init__.py
│   │       ├── benchmark-results
│   │       │   └── ...
│   │       ├── data
│   │       │   ├── EXAMPLES.markdown
│   │       │   ├── exampledatafile
│   │       │   └── group-emails-example
│   │       └── test_suite.py
...
FriedrichFroebel commented 8 months ago

As already said and mentioned inside the pytest docs as well: This depends on the preferred distribution model and some other factors. I tend to lean towards a strict separation of binary distributions (wheels) with the actual application code only and a separate source distribution (sdist) with all the source files (including tests) to use for building own wheels, OS-specific packages etc.

Shipping the tests inside a dedicated package makes the actual installation from wheels smaller as it does not have to ship the testing stuff - this is the preferred way for libraries which are used in larger projects etc. where we can assume that the package itself is well-tested in an isolated manner.

Shipping the tests inside the actual package tends to bloat the wheels and only makes sense if you want to run the tests from within a larger application, which does not make much sense in most of the cases.

Sann5 commented 8 months ago

@FriedrichFroebel I understand. I restored the original project structure. Let me know if there is anything else that doesn't check out.

harelba commented 8 months ago

I believe that i'm using an old ubuntu version for the github actions build/test workflow, and therefore some of the workflow jobs are infinitely queued without running.

I will try to push some changes to the github actions to see if i can make them run properly.