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 contribution guidelines #71

Merged
merged 22 commits into from
May 12, 2021
Merged

add contribution guidelines #71

merged 22 commits into from
May 12, 2021

Conversation

rkurchin
Copy link
Member

Open to any feedback on this stuff! Does the list of tags look good? Am I missing anything?

(obviously still a work in progress, placeholders should be obvious)

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2021

Codecov Report

Merging #71 (57fdfc4) into main (c15f288) will decrease coverage by 0.98%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #71      +/-   ##
==========================================
- Coverage   67.16%   66.17%   -0.99%     
==========================================
  Files           2        2              
  Lines          67       68       +1     
==========================================
  Hits           45       45              
- Misses         22       23       +1     
Impacted Files Coverage Δ
src/models.jl 25.00% <25.00%> (-2.28%) ⬇️
src/layers.jl 75.00% <76.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c15f288...57fdfc4. Read the comment docs.

@rkurchin rkurchin linked an issue Apr 30, 2021 that may be closed by this pull request
@rkurchin rkurchin requested a review from thazhemadam May 10, 2021 20:33
* Keep things modular! If you are fixing/adding multiple things, do so via separate issues/PR's to streamline review and merging.
* It is recommended that you set up [JuliaFormatter](https://domluna.github.io/JuliaFormatter.jl/dev/) (also see [VSCode extension](https://marketplace.visualstudio.com/items?itemName=singularitti.vscode-julia-formatter)). A GitHub action will check that code adheres to the style guide.

## Getting Started
Copy link
Member

Choose a reason for hiding this comment

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

How necessary is this in CONTRIBUTING.md? I'm assuming that stuff like running git clone ..., activating a Julia environment, etc would be mentioned here.
But wouldn't the people who would be looking at this file already have the essential know-how of doing all that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the answer to this depends upon how many people you think are out there who can write decent Julia code, might be interested in contributing some of their code to a package, but haven't actually developed packages before. Given that I was such a person relatively recently (and was one in Python a couple years before that), I tend to think there are a fair number, but I may be biased due to personal experience... ;) At any rate, I don't think it hurts to have it there...

Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than actually writing a new guide, I decided to just compile some other nice resources.

@@ -12,10 +12,8 @@ Documentation is in progress [over here](https://aced-differentiate.github.io/At

2. Go and try out the example in examples/example1/ – it has its own README file with detailed 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 it might help to put a quick note on how to actually use the package here (like ] add AtomicGraphNets, and variants).

@@ -10,18 +10,17 @@ using Random
using ChemistryFeaturization
using AtomicGraphNets
using Serialization
<<<<<<< HEAD
<< << << < HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Should this be here? Looks like this was overlooked while resolving a merge conflict...

Copy link
Member Author

Choose a reason for hiding this comment

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

haha yuuup thought I'd fixed that, oops

@rkurchin rkurchin requested a review from thazhemadam May 12, 2021 14:04
README.md Show resolved Hide resolved
@rkurchin rkurchin merged commit b74c1a3 into main May 12, 2021
@rkurchin rkurchin deleted the contrib_guide branch May 12, 2021 19:59
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.

Set up community guidelines and standards
3 participants