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

adding contributors guidelines #580

Merged
merged 9 commits into from
Jan 16, 2024

Conversation

alex-rakowski
Copy link
Collaborator

This PR adds CONTRIBUTORS.md and updates README.md to point to it.

Creating a new PR as the previous seems to have gotten lost in a merge

@sezelt
Copy link
Member

sezelt commented Dec 12, 2023

In my opinion, we should not make conda environments optional in the instructions.

We may want to add some discussion about adding dependencies. Perhaps not too much detail (we can save that for individual PR discussions) but mainly just to mention that py4DSTEM is distributed via pip and conda-forge, so any dependencies must be available from both sources.

I think it has not really been our practice within the team to open issues before PRs. I do think it is a good thing for first-time contributors to do, though.

@gvarnavi
Copy link
Member

Looks good to me overall, thanks @alex-rakowski!

Quick note, following @sezelt point: I'm actually a fan of draft PRs instead of opening issues we reference in an eventual PR. It seems a more natural/contained way to outline ideas for a PR and/or solicitate feedback about how to implement something. If the contributors already have code, it would be much easier to talk about it rather than abstractly in an issue, but they can also be empty as far as I understand, if the objective is simply to brainstorm. From the github blog introducing them:

But what if you want to signal that a pull request is just the start of the conversation and your code isn’t in any state to be judged? Perhaps the code is for a hackathon project. You have no intention of ever merging it, but you’d still like people to check it out locally and give you feedback. Or perhaps you’ve opened a pull request without any code at all in order to get the discussion started.

@alex-rakowski
Copy link
Collaborator Author

In my opinion, we should not make conda environments optional in the instructions.

I will remove the optional part

We may want to add some discussion about adding dependencies. Perhaps not too much detail (we can save that for individual PR discussions) but mainly just to mention that py4DSTEM is distributed via pip and conda-forge, so any dependencies must be available from both sources.

So something like ~ adding dependencies should be done thoughtfully. They must not have restrictive install requirements, be well maintained and available on both pip and conda. ?

I think it has not really been our practice within the team to open issues before PRs. I do think it is a good thing for first-time contributors to do, though.

I guess this was a personal preference that we should start adding issues, even if they're brief, particularly for bug fixes, install issues etc. We discuss issues and features in slack and dev meetings, but this would make them searchable.

@alex-rakowski
Copy link
Collaborator Author

Looks good to me overall, thanks @alex-rakowski!

Quick note, following @sezelt point: I'm actually a fan of draft PRs instead of opening issues we reference in an eventual PR. It seems a more natural/contained way to outline ideas for a PR and/or solicitate feedback about how to implement something. If the contributors already have code, it would be much easier to talk about it rather than abstractly in an issue, but they can also be empty as far as I understand, if the objective is simply to brainstorm. From the github blog introducing them:

But what if you want to signal that a pull request is just the start of the conversation and your code isn’t in any state to be judged? Perhaps the code is for a hackathon project. You have no intention of ever merging it, but you’d still like people to check it out locally and give you feedback. Or perhaps you’ve opened a pull request without any code at all in order to get the discussion started.

I agree that most of the discussion around code implementation can/should happen in the PR as that feels where it belongs.

But I (and I think others) would go to the issues page if I'm having trouble installing or getting a plotting error etc., as it feels more natural to check issues than to search PRs. If we stick with PRs only, we should probably start labeling them and/or following some of the styles of semantic PRs.

I feel like a workflow like below would be searchable and keep everything in the semantically correct place:

Bugs and Issues

# open issue
py4DSTEM.show_complex produces an error when plotting probe from OverlapTomo when device="GPU"

py4DSTEM version: 0.14.5

# More complex errors have more details, steps to reproduce, etc. 
# Draft/PR to discuss and ultimately fix it
This PR addresses issue 499
error caused because of ...
# close issue
PR 500 fixes this, and will be included in the next release 0.14.6., 
For an immediate fix install dev branch or call `py4DSTEM.show_complex(np.array(probe))` 

New Features

# open Draft PR
This PR is to discuss adding a new feature ... 

I guess both of these should then go into a changelog or release note for best practice but that would be a separate discussion / Issue / Draft-PR.

@sezelt
Copy link
Member

sezelt commented Dec 12, 2023

I'm not saying we should stop using issues and only make PRs. The point is just that if you are proposing to make edits to the code you should make a PR and not an issue.


### Code of Conduct

py4DSTEM is committed to providing a welcoming and inclusive environment for all contributors. Please read and follow the py4DSTEM Code of Conduct.
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to reference this we probably need to add it!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Copy link
Member

@bsavitzky bsavitzky left a comment

Choose a reason for hiding this comment

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

All looks good to me, thanks @alex-rakowski . I made some edits, please let me know if you think any of them should be changed or reverted.

The only outstanding issues I see are

  1. resolving the question of asking for filing an issue vs. draft PRs when new contributors make PRs (see my comment on that discussion), and
  2. we reference a Code of Conduct in this document, but have not added that file yet

This is a suggested template that seems widely used and seems reasonable.
@alex-rakowski
Copy link
Collaborator Author

alex-rakowski commented Jan 12, 2024

@bsavitzky I added a code of conduct, it's a suggested template that seems reasonable. It lists you as the point of contact for any issues, I presume that's fine?

@bsavitzky
Copy link
Member

@bsavitzky I added a code of conduct, it's a suggested template that seems reasonable. It lists you as the point of contact for any issues, I presume that's fine?

Perfect, yes! Thanks Alex, looks good.

@bsavitzky bsavitzky merged commit 12c7c02 into py4dstem:dev Jan 16, 2024
7 checks passed
bsavitzky added a commit to bsavitzky/py4DSTEM that referenced this pull request Mar 12, 2024
adding contributors guidelines

Former-commit-id: 12c7c02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants