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

add CONTRIBUTING doc #152

Merged
merged 3 commits into from
Nov 22, 2024
Merged

add CONTRIBUTING doc #152

merged 3 commits into from
Nov 22, 2024

Conversation

xiangchenjhu
Copy link
Contributor

@xiangchenjhu xiangchenjhu commented Nov 1, 2024

This PR addresses the documentation issue described in #151.

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@xiangchenjhu xiangchenjhu mentioned this pull request Nov 1, 2024
@xiangchenjhu xiangchenjhu marked this pull request as ready for review November 1, 2024 14:05
@xiangchenjhu xiangchenjhu requested a review from a team as a code owner November 1, 2024 14:05
Copy link
Member

@jamienoss jamienoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to add this.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
Comment on lines 77 to 78
git add .
git commit -m "Description of changes"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git add . is not a good idea either. git commit -a will auto add changes to already tracked files.

Suggested change
git add .
git commit -m "Description of changes"
git add <path-to-newly-added-file-if-applicable>
git commit -am "Description of changes"

CONTRIBUTING.md Outdated
To adapt this `CONTRIBUTING.md` template for a specific project:
1. Replace `[PROJECT_NAME]` with the name of your project.
2. Replace `[REPOSITORY_URL]` with the URL of the main repository on GitHub.
3. If needed, replace `[FORMAT_TOOL]` with the specific formatting tool or linter used in your project (e.g., `ruff`, `black`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this line since we hide the tool behind tox.

CONTRIBUTING.md Outdated

Thank you for your interest in contributing to [PROJECT_NAME]! As a contributor, you’ll work from your fork of the main repository. This document outlines the steps to set up your development environment, guidelines for coding, and instructions for submitting contributions.

## Customization Instructions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this entire section and hard-bake package_name and ssec-jhu/base-template such that project_setup.py can auto-correct these.
@ryanhausen that should work, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I’ve hard-baked those two values. Note that this also requires updating project_setup.py to include processing for CONTRIBUTING.md.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be already accounted for here:

def valid_file_predicate(file_path: Path) -> bool:
    """Checks if a file is valid for modification."""
    is_valid_extension = file_path.suffix not in [
        # images
        ".png",
        ".jpg",
        ".jpeg",
    ]
    not_ds_store = file_path.name != ".DS_Store"
    not_this = file_path.name != Path(__file__).name

    return is_valid_extension and not_ds_store and not_this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanhausen can you confirm this, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Just follow the convention used for the placeholders in other files and it should get replaced automatically.

This was referenced Nov 12, 2024
Copy link
Contributor Author

@xiangchenjhu xiangchenjhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the great feedback! I’ve made updates based on your suggestions. Please review it again.

Copy link
Member

@jamienoss jamienoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just 2 tiny changes regarding the project name vs repo name.

CONTRIBUTING.md Outdated
@@ -28,35 +19,33 @@ After making these replacements, this file will be ready for use in your project
3. **Clone Your Fork Locally**:
- Once your fork is created, you can clone it to your local machine to start working:
```bash
git clone https://github.com/your-username/[PROJECT_NAME].git
cd [PROJECT_NAME]
git clone https://github.com/your-username/package_name.git
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be the repo url, not the package name.

CONTRIBUTING.md Outdated
Comment on lines 22 to 23
git clone https://github.com/your-username/package_name.git
cd package_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here this is the repo name, not the package name, however, it might be easier to just leave this one as something like: cd <repo dir>. @ryanhausen is there something in project_setup.py that will pick up just the repo dir name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, use repo dir enhance flexibility!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks! But for clarity, it's not a point of flexibility, but that you used the repo URL and repo names don't have to match project/package names. So if/when they differ, this would break.

Copy link
Contributor Author

@xiangchenjhu xiangchenjhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the thoughtful review! I've revised the changes accordingly. Please take another look.

Copy link
Member

@jamienoss jamienoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jamienoss jamienoss added the documentation Improvements or additions to documentation label Nov 22, 2024
@jamienoss jamienoss merged commit 5d0ad2f into main Nov 22, 2024
8 checks passed
@jamienoss jamienoss deleted the add-contributing-doc branch November 22, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants