-
Notifications
You must be signed in to change notification settings - Fork 198
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
Python package #267
Python package #267
Conversation
@JoshData friendly ping |
1 similar comment
@JoshData friendly ping |
Hey, Sorry I'm actually confused by the changes. If one follows the README steps, I think you will get a copy of the library installed in the usual Python packages location, but then it says to run "./run" which will run the version that has been cloned. Or will it? Will it get the modules in the working directory or will it go to the installed package? |
If we following the README as is, |
I don't think this is a good idea... |
Do you mean to say that the documentation should be changed to Just to clarify |
The documentation needs to present a clear way to use the package, so it has to either use an installed package or not install it. (It shouldn't install a package but then not use it.) It shouldn't create two local copies of the package either (the git clone and then the one installed by pip). And "run" is a bad name for a globally-installed script. |
Thats sensible, I'll change the local
This is a harder one to tackle. As the only way to install the package is to clone it first, unless you are using a package manager. There is no way around this unless you symlink the two with
Technically it will only work in the virtual env the package is installed, but yeah. Totally agree. Do you have a proposition for a better name? |
@JoshData any further thoughts?
|
Is this PR solving a real problem that you are having, or is this just for fun? |
I think I have outlined the real world problems this resolves in the OP. For most users however, this brings us one step closer to resolve #188, i.e. able to install unitedstates/congress with a I would very much like for this PR to be merged in as I believe it is useful. I just suggested the second point as ultimately I don't want to keep pushing this unless you believe there is merit in this PR, since I should respect your wishes as being the current maintainer of this project. |
@JoshData, @acxz I also think that having this as a Python package is helpful. As a general principle, I've moved to having all of my Python scripts as packages so that I can call their modules without remembering the paths or setting environment variables. In this case I think @JoshData has a valid concern about namespacing. Using This may be disruptive to current users, but I think it provides more value than being able to call the If @JoshData is open to the more significant change above, I would suggest a few updates (in both the code and documentation):
|
That was actually on my to do once the setup.py is integrated in this PR. Wanted to take it a step at a time ;) Points 1 and 2 should be addressed in a follow up PR. I'll go ahead and address Point 3 in this PR. Thanks for your input @aih! |
hmm turns out I can't alias the Otherwise, personally I think this PR should be merged in as it is, and we can address @aih's concerns in a later (possibly more disruptive, in terms of file structure, PR). Just getting a @JoshData if you could let us know if you are still interested in this feature that would be appreciated. |
Definitely useful to have this set as a package:
I agree with @JoshData for the PR itself; I don't think it's a good idea to include the run script, or a "command" script at all. The modules separated as is is good enough for people to simply import (e.g. the vote.py file is meant for vote scraping, and that's it, just like firebase admin); it'll just need extra documentation. +1 on making this a package though! |
With the latest commit this is taken care of. |
To really make the python methods and modules importable, I tried to create the setup.py file so that the directory structure could be kept the same. However, after spending quite a bit of time I wasn't able to get it to work. I decided to follow python packaging convention (this involves moving files into a With this @aih's concerns are addressed and with a Associated doc changes have been added as well. I would urge anyone interested in this endeavor to test out the package for their particular use case. Please feel to critique and state any concerns! |
change directory structure to make python package conventional add setup.py file to specify deps guide users to use the installed `usc-run` command associated changes to other scripts make scripts installable when package is installed add a symlink for congress/run.py to run for backwards compat remove redundant requirements file
This seems OK to me. Would be great if someone else could try it out next. |
pinging @stevesdawg @Andrew-Chen-Wang @aih If you guys can test it out for your use case and report back if it works or doesn't please do so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if the package was versioned by date. It makes more sense as a scraper of a constantly changing website.
I also have stopped using this repo for work, but scrapers are in need all the time so please do merge! Thanks again for the lovely work:)
Thx for the review!
That actually makes a lot of sense. I'll add the change depending on @JoshData's opinion. |
None at all |
@aih things thatre worth testing seems like the CLI command, general importing of the package, and a single use case |
The test plan I ran for these changes, from inside the repo root:
These worked normally as expected! 🎉 One side-effect however, when I call What are people's thoughts on this? |
I agree with you, but I think that can be a subsequent PR and need not block merge. Right now, you can only really run from within the repo. |
If you made a system install and implemented something where the folders are created at "top level", you wouldn't see the folders at all anymore since they'd be next to or even inside the package content (ie wherever the environment is). I think this behavior is desirable. |
Command line importing looks good 👍🏾
This looks good to me. |
@JoshData it has been some time, since this PR has been tested by folks here. LGTM? |
Thanks all! |
@JoshData I'm back haha, (This might be my last one in a while)
This patchset allows users to install congress as a python package. This offers several benefits:
dependencies: can be directly specified in the setup.py instead of having a requirements file. This ensures that users who want to use congress will have the requirements instead running an extra command.
exposing entry points: the main
run
andtest/run
files can be run anywhere as long as your python environment is the same one where you installed congress in, providing more convenience to users say in writing their own shell scripts without hardcoding the congress directory. Of course you can still use./run
andpython run
as before, so documentation does not need to be changed.Makes it easier for package managers to work with. While this does not directly reslve PyPi package available? #188 if someone wants to, one can easily upload congress to PyPi now, saving users the step to even clone the repo. Distributions that currently package this codebase, such as Arch Linux, currently manually install the scripts to the usr/bin folders. (https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=congress-git) Now they can use built in python build instructions such as
python setup.py install
.If this is merged, this can fixes #192