Add CONTRIBUTING file #86
Conversation
| python -m venv .venv | ||
| source .venv/bin/activate # On Windows: .venv\Scripts\activate |
There was a problem hiding this comment.
Awesome seeing you adapt to venv, thanks for your flexibility with this once again <3
I'd modify this to:
python -m venv ~/.virtualenvs/ibind
source ~/.virtualenvs/ibind/bin/activate # On Windows: ~/.virtualenvs/ibind/Scripts/activate.bat
While naturally what you suggested would work, I think this global .virtualenvs folder is the standard way of doing this.
Also, is source the right command here, shouldn't it be . or sh for linux?
There was a problem hiding this comment.
So this is interesting! I didn't respond immediately to this because I haven't done a ton of python projects and wanted to understand why I defaulted to project based venv folders, other than poetry's convention.
Open to being corrected here and open to all input as always 😁 - Here's my current understanding:
The approach recommended by the standard library's venv module is to create a .venv directory directly within the project. This keeps everything self-contained and makes setup straightforward when cloning the repository.
I initially took this from Python packaging guide: https://packaging.python.org/en/latest/guides/installing-using-pip-and-virtual-environments/#create-a-new-virtual-environment
You mentioned global venvs, and wanted to understand where that's from. It seems like that was popularized by the virtualenvwrapper tool which defaults to storing environments centrally in ~/.virtualenvs
This appears to come from the virtualenv package here https://virtualenv.pypa.io/en/latest/user_guide.html
Happy to dive deeper, if we're using the python standard for venv, it seems the convention is project scoped. I guess it's not a breaking convention either way, but for new contributors I think it would be nice to align on the same way :)
Regarding the activation cmd:
My understanding source is correct and standard command for bash and zsh shells. It executes the script's commands within the current shell, which is necessary to modify the environment variables (like PATH).
My thought is that using sh activate would be incorrect, as it would run the activation script in a subshell. The environment changes made in that subshell wouldn't persist back to the user's main shell, defeating the purpose of activating the virtual environment for the current session.
There was a problem hiding this comment.
Superb reply @weklund - I was not aware that .virtualenvs isn't considered standard anymore. Thanks for digging into this and reporting back in detail. I'll continue setting up with .virtualenv as it makes it a tad easier for me when I back up projects to a hard drive - copying all venv packages would be a waste, kinda like node_modules - which addmittedly happens very rarely, this is hardly a benefit. That said, I think it's wise to recommend using venv like you originally suggested, great job defending that point 👏
As for the activation cmd, gotcha, thanks for clarifying that too.
Consider this point resolved, thanks for your input
| - Describe your proposed solution | ||
| - Include use cases and benefits | ||
|
|
||
| ## Code Style |
There was a problem hiding this comment.
should we maybe add 'use ruff linter/formatter` here?
There was a problem hiding this comment.
Great to see the addition, but I think the existing lines were great too:
- Follow PEP 8 guidelines
- Write docstrings for all public functions and classes
- Keep functions focused and single-purpose
- Use meaningful variable and function names
How about we keep both?
| - Update examples if applicable | ||
| - Keep the README.md up to date | ||
|
|
||
| ## Release Process |
There was a problem hiding this comment.
Do we need this section? What would you envision putting here?
There was a problem hiding this comment.
My thinking was just a sentence or 2 describing how releases are cut. I'm imaging/assuming something like, "Semver versioning is used" and describe how to get test releases like you did with the oauth work. Just from looking at the repo it's a bit unclear what the current process is.
There was a problem hiding this comment.
Gotcha, how about something along the lines of:
IBind is distributed through Pypi.
Semantic Versioning 2.0.0 is used.
Minor changes are published directly as new releases. Otherwise one or many release candidate versions are published (eg. `rc1`, `rc2`, etc.) and made available for a period of time in order to test the new functionalities.
describe how to get test releases
Does that address this point sufficiently enough? Would you envision anything else here?
|
hey @weklund just to follow up on this - let me know if you'd like me to update the remaining two points after the merge or if you're thinking to do it yourself when you find a minute. Happy to go either way 👍 |
|
I think this is pretty much ready to merge, anything else left to address? |
|
Nope I think that's it! Thanks for the all the feedback. |
|
Great job 👏👏 |
@Voyz We mentioned adding this a while back, figured let's get one to at least look at and see what we think. Open to any and all feedback! :)