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

[PR]: Simplify the contributing guide #593

Merged
merged 11 commits into from
Jun 5, 2024
Merged

Conversation

tomvothecoder
Copy link
Collaborator

@tomvothecoder tomvothecoder commented Jan 30, 2024

Description

Todo list

  • Add Overview section
  • Add Where to start? section
    • Documentation updates
    • Bug reports
    • Enhancement requests
    • All other inquiries
  • Add Version control, Git, and GitHub section
    • Getting started with Git
  • Add Creating a development environment section
    • Install pre-commit hooks
  • Add Contributing to codebase section
    • Pull Request (PR)
    • Code Formatting
    • Testing with Continuous Integration
    • Writing Tests
  • Add Developer Tips section
    • Helpful commands
    • xCDAT and Visual Studio Code

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

@tomvothecoder tomvothecoder added the type: docs Updates to documentation label Jan 30, 2024
@tomvothecoder tomvothecoder self-assigned this Jan 30, 2024
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (07bfbba) to head (57a9a00).

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.
📢 Have feedback on the report? Share it here.

@tomvothecoder tomvothecoder changed the title Add initial updates to contributing guide [Doc]: Simplify the contributing guide Jan 31, 2024
@tomvothecoder
Copy link
Collaborator Author

tomvothecoder commented Jan 31, 2024

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 pre-commit, re-organized and simplified sections such as setting up a dev env, and made it more friendly sounding (less stringent guidelines and standards).

@tomvothecoder tomvothecoder changed the title [Doc]: Simplify the contributing guide [PR]: Simplify the contributing guide Feb 12, 2024
@tomvothecoder
Copy link
Collaborator Author

tomvothecoder commented Feb 16, 2024

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.

Copy link
Collaborator

@pochedls pochedls left a 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.

@tomvothecoder
Copy link
Collaborator Author

tomvothecoder commented Apr 2, 2024

@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).

@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 mamba references back to conda.

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.

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.

@tomvothecoder
Copy link
Collaborator Author

@pochedls Just bugging you again for your approval

Copy link
Collaborator

@pochedls pochedls left a 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 with repository
  • 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.

@tomvothecoder
Copy link
Collaborator Author

tomvothecoder commented Jun 5, 2024

Additional item from today's meeting:

  • fix tagging of GH issues for beginners to work on -- I tagged all of the "docs" issues as good-first-issue. I'll tag a few enhancements too.

@tomvothecoder tomvothecoder added the good-first-issue Good first issue for new contributors label Jun 5, 2024
@tomvothecoder
Copy link
Collaborator Author

tomvothecoder commented Jun 5, 2024

  • Doesn't the first code block do (1) and (2)? Should the following be outside the code block?:

(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 xcdat into the dev environment since we don't include the stable version of xcdat in the dev env (e.g., xcdat=0.7.0). (2) allows the user to test/try code changes in the env.

@pochedls
Copy link
Collaborator

pochedls commented Jun 5, 2024

Doesn't make install # or python -m pip install . in the code block under 1. Install the build dependencies actually build/install xcdat?

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.

@tomvothecoder
Copy link
Collaborator Author

Doesn't make install # or python -m pip install . in the code block under 1. Install the build dependencies actually build/install xcdat?

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.

@tomvothecoder
Copy link
Collaborator Author

I addressed the suggestions and will merge this PR now.

@tomvothecoder tomvothecoder merged commit 223869e into main Jun 5, 2024
9 checks passed
@tomvothecoder tomvothecoder deleted the update/contributing-guide branch June 5, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good first issue for new contributors type: docs Updates to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Doc]: Simplify the contributing guide
2 participants