-
Notifications
You must be signed in to change notification settings - Fork 12
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
[PR]: Simplify the contributing guide #593
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #593 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 15 15
Lines 1542 1542
=========================================
Hits 1542 1542 ☔ View full report in Codecov by Sentry. |
Hey @xCDAT/core-developers, here is my initial PR draft to simplify the contributing guide. It is based on Xarray's contributing guide, but geared towards xCDAT and made even simpler. If any of you are available for a quick review, that would be great. You can review the page directly in this Read The Docs preview: https://xcdat.readthedocs.io/en/update-contributing-guide/contributing.html. Thanks! Here is the old version if you wanted to compare against it: https://xcdat.readthedocs.io/en/v0.6.0/contributing.html. I removed overly technical sections about version control/Git and |
Hi @chengzhuzhang, just tagging you for review on this PR. It's not urgent, so whenever you have time for it that would be great. |
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.
@tomvothecoder – I did make some changes locally (from my forked repo). I basically just deleted mamba stuff (conda now uses the mamba solver by default).
I had some git setup steps (I tried to start from scratch), e.g.,:
git config --global user.name “…”
git config --global user.email “…”
But I think that can be beyond the scope of our developer guide. We may need to help some of our users with git stuff.
7dab28b
to
2c1c766
Compare
@pochedls Thanks for the reminder about this update to Conda's default solver. In this commit I added your suggestions and updated the Installation page to change
I added a section on setting up the repo by either cloning or forking (this commit). My thinking was that adding git content would make the contributing guide lengthy. From my experience, the basic commands are easy to look up (configs, cloning, fetching, pulling, pushing, and rebasing). As an alternative, we can help individual contributors directly if they have issues with Git. |
@pochedls Just bugging you again for your approval |
6b5d6c4
to
2d00d03
Compare
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.
Looks good. A couple minor comments:
- Consider replacing
repo
withrepository
- This section might have issues.
- Doesn't the first code block do (1) and (2)? Should the following be outside the code block?:
- This will create the new environment, and not touch any of your existing environments, nor any existing Python installation.
Additional item from today's meeting:
|
(1) does not do (2). (1) Creates the dev environment with all of the dependencies required xCDAT, while (2) builds and installs the local version of |
Doesn't It seems like the code block under 2. Build and install xcdat simply ensures that it was built correctly and shows up as a loadable module (i.e., it does not build/install xcdat, which was done under buller 1)? Note that I am looking at this rendered page. |
Ohhh I see what you're saying. (2) should be labeled "Import the local build of xCDAT". Thanks for catching this. |
1b44ffd
to
57a9a00
Compare
I addressed the suggestions and will merge this PR now. |
Description
Todo list
Checklist
If applicable: