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

#28 contributing guide #114

Merged
merged 6 commits into from
Jul 7, 2020
Merged

#28 contributing guide #114

merged 6 commits into from
Jul 7, 2020

Conversation

williballenthin
Copy link
Collaborator

i derived a lot of the structure and text from https://github.com/atom/atom/blob/master/CONTRIBUTING.md

closes #28

@williballenthin williballenthin added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 6, 2020
@williballenthin
Copy link
Collaborator Author

imho, i recommend that we merge and then any/all make edits for the next week. for example, i'd like to see more details about setting up a development environment and doing an initial checkout. maybe also move text from the installation guide into the contributing guide.

Copy link
Collaborator

@mr-tz mr-tz left a comment

Choose a reason for hiding this comment

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

looks great, thanks!

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
@williballenthin williballenthin merged commit 758ee87 into master Jul 7, 2020
@williballenthin williballenthin deleted the contributing-guide branch July 7, 2020 15:06
All Python code must adhere to the style guide used by capa:

1. [PEP8](https://www.python.org/dev/peps/pep-0008/), with clarifications from
2. [Willi's style guide](https://docs.google.com/document/d/1iRpeg-w4DtibwytUyC_dDT7IGhNGBP25-nQfuBa-Fyk/edit?usp=sharing), formatted with
Copy link
Member

Choose a reason for hiding this comment

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

@williballenthin has an style guide! 😮 Two thoughts:

  • Aren't some of those covered by black and isort?
  • Maybe it is a good idea to move those to a gist/GitHub repo to link them easily.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, some of my style guide is covered by isort/black, especially on the formatting end.

other things, like "function should return a single type" don't get enforced (though this one might by via mypy).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'm open to moving this style guide into gh. we can copy it into the capa project and call it the "capa style guide" rather than use my name?

@Ana06
Copy link
Member

Ana06 commented Jul 7, 2020

Really good documentation! 👍 Impressive! 😻

Prefix the first line with the component in question ("rules: ..." or "render: ...")

Can we have some more information about this? I have seen @williballenthin using it and I find it a good idea. But I am not sure which component we have and which one I should use if I touch more than one. I think that some more information about this would be helpful.

@Ana06
Copy link
Member

Ana06 commented Jul 7, 2020

Do we have documentation about how to link the tests? Otherwise I think it is a good idea to at least link the ci file where the tests are run, so that contributors can easily figure out how to run tests locally.


<!--

Have you read Capa's Code of Conduct? By filing an Issue, you are expected to comply with it, including treating everyone with respect: https://github.com/fireeye/capa/blob/master/CODE_OF_CONDUCT.md
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 should include here also a link to the Suggesting Enhancements secction of the CONTRIBUTING file.

I also think we should mention here (and in the bug report) that issues related to rules should be created in capa-rules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point

@williballenthin
Copy link
Collaborator Author

Prefix the first line with the component in question...

to be completely honest, i've tried to avoid thinking too hard about this. if you follow a string of my commits, you'll see that i'll using different spelling, names, etc. one after another cause i forget what i've used before. its less about being programmatically accessible and more about:

  1. hinting to a human what the commit is about
  2. keeping the commit focused on a single thing, since it usually has to fit in a single prefix

do you think we should predefine a bunch of components? what kind of further specification would be helpful?

@williballenthin
Copy link
Collaborator Author

Do we have documentation about how to link the tests?

definitely would like to get more documentation into these files. this PR is just a first pass. please go ahead and add text to address any missing items.

imho, i think we should all just commit to master on these docs to get them up to shape, rather than commit-review-merge-repeat.

@Ana06
Copy link
Member

Ana06 commented Jul 8, 2020

Prefix the first line with the component in question...

to be completely honest, i've tried to avoid thinking too hard about this. if you follow a string of my commits, you'll see that i'll using different spelling, names, etc. one after another cause i forget what i've used before. its less about being programmatically accessible and more about:

1. hinting to a human what the commit is about

2. keeping the commit focused on a single thing, since it usually has to fit in a single prefix

do you think we should predefine a bunch of components? what kind of further specification would be helpful?

I am not sure if we should. If we don't want to enforce it, maybe it is not needed. Maybe adding a few examples is enough. 🤔

This pull request was closed.
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a CONTRIBUTING file
3 participants