-
Notifications
You must be signed in to change notification settings - Fork 53
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
doc: make template more concise, add ref to CONTRIBUTING.md on template #357
doc: make template more concise, add ref to CONTRIBUTING.md on template #357
Conversation
5e607f2
to
12896a2
Compare
Nack. The positional changes just makes worse to make a superficial read of the PR.
No, its not... This project has more code than quantity of people reviewing it and the section
Description is an open text field and the understanding, of the contributor, about whats being addressed can differ depending on how familiar the contributor is with the codebase... Also, most contributors, as you can see from the contribution history, are not native English speakers and this can lead to difficulties in writing and understanding for both contributor and reviewer. So, check boxes makes life easier for everyone. |
Do you mean from the reviewer perspective? Because if not, I think that if someone makes a PR they should be able to explain their work.
Google translate is more than enough for this. |
I think there are some nice changes here. Could you bring this section back and not remove the sample title from issues? |
12896a2
to
f0c8fe3
Compare
@Davidson-Souza do you mean the "Which aspect of floresta its being addresed?" section? |
Yes |
f0c8fe3
to
2c7783b
Compare
Done. I added the individual crate names: +### Which crates are being modified?
+
+- [ ] floresta-chain
+- [ ] floresta-cli
+- [ ] floresta-common
+- [ ] floresta-compact-filters
+- [ ] floresta-electrum
+- [ ] floresta-watch-only
+- [ ] floresta-wire
+- [ ] floresta
+- [ ] Other: <!-- Please describe it -->. |
Its totally possible to contribute without knowing all the project in details, isnt it ? I understand, the point is to make the template more concise but, few lines with check boxes will not hurt anyone and will help reviewers to provide better and faster reviews.
Nice but, "Which aspect of floresta its being addresed?" is not about the crates and yes about the functionality being changed... Still a working in progress to separate things in their proper crates here. Also, I see a way to make it more concise to complement the objective of this PR. What you think about: + ### Which aspect of LibFloresta its being addresed?
+ - [ ] Blockchain; <!-- Block validation and management. -->
+ - [ ] Wire; <!-- Communication and control over other nodes. -->
+ - [ ] I/O or Database <!-- Control over the data flow for the OS. -->
+ - [ ] Other: <!-- Please describe it -->. Smaller but still expressive. |
I think I prefer the crate names. Some projects (like rust-bitcoin) even have some automation to tag which crates are changing on a PR. ACK 2c7783b |
But we can know which crates were changed by the git diff... So no extra section is needed at all |
The same could be said about the aforementioned example. These information are meant to be read before doing any git stuff (otherwise I can figure out myself) |
Indeed |
2c7783b
to
c7756d1
Compare
Kinda... The objetive that i had in mind for this section was to understand how much the PR can be dangerous and what feature will be aftected by i and i dont believe that showing the which crates was changed can tell me that... BUT, i saw some people oppening PRs using this section and no one used it properly, at least not in the way i was expecting when i proposed this, which makes the whole section useless. Marking by crates is certainly more intuitive. ACK c7756d1 |
What is the purpose of this pull request?
Which crates are being modified?
.github/
Description
Made the PR template cleaner: the description section should be enough (
Which aspect of floresta its being addresed?
is redundant).Added CONTRIBUTING.md to checklist section
Fixed typos on CONTIBUTING.md
Checklist
just lint
cargo test
PS: this PR is using the new template