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 docs for contributing and private items #26

Merged
merged 9 commits into from
Jun 14, 2023

Conversation

AucaCoyan
Copy link
Contributor

@AucaCoyan AucaCoyan commented Jun 11, 2023

Hello! I took some time between my last PR and this one because I was documenting everything on the code 😍 .

This is the list of contributions, I would like to hear the thoughts of @fdncred and @amtoine about them before merging this PR:

  • docs/CONTRIBUTION.md added. I explained the why and the how to contribute, and appended a list of general guidelines about the philosophy of the project:
  • Everything should be explained: rust docs, comments, drawings, pick what makes you comfortable, but it is important to make it clear. There will always be some new guy or gal into the project we want to welcome 😄.
  • Use clear variable names and try to avoid confusing abbreviations. Think that your peers may not be fully fluent in english 💬.
  • I added a few clippy lints from clippy:pedantic, here are them: Clippy will give a warning if one if them is not compliant.

I added these rules because I would like to improve the quality of upcoming contributions. I'm not sure if this is asking too much from the people who want to be involved in the project, as some of the rules may be a bit annoying. Do you see it doable? Do you think we should delete some or all of the clippy lints?

@AucaCoyan AucaCoyan marked this pull request as ready for review June 11, 2023 22:01
Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

I'm generally good with this. Some of the doc requirements may be overkill, but for now, I'm ok with these changes.

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

i like that 😊

just a few little remarks and suggested changes and we're good to go ✨

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
src/formatting.rs Outdated Show resolved Hide resolved
src/formatting.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@fdncred
Copy link
Collaborator

fdncred commented Jun 12, 2023

Maybe we should add a nufmt channel in discord now that we're being more official with this now? I just mention here because it may change some links referenced.

@amtoine
Copy link
Member

amtoine commented Jun 12, 2023

Maybe we should add a nufmt channel in discord now that we're being more official with this now? I just mention here because it may change some links referenced.

oooh yeah, we have a thread only 👍
if we create a channel, the link might have to change as you said 👌

@AucaCoyan
Copy link
Contributor Author

AucaCoyan commented Jun 12, 2023

Maybe we should add a nufmt channel in discord now that we're being more official with this now?

I would love to! Tag me if you create it.
I have many things to reply, but it is going to be a long conversation if we don't do it on chat 🤣

@amtoine
Copy link
Member

amtoine commented Jun 13, 2023

@fdncred
link to the new #nufmt channel updated in 13ae3b0 👌

@AucaCoyan
Copy link
Contributor Author

I believe I addressed all the changes you requested, but feel free I you find some more or want to redo something!
@amtoine

actually there are link *variables* at the bottom of the document
which makes it easier to read i think and the link to the `#nufmt`
channel is wrong:
- https://discord.com/channels/601130461678272522/1117921521520873623 is
  the new `#nufmt` channel
- https://discord.com/channels/601130461678272522/1087437882396004442
  was the link of the closed `#nufmt` threads under the `#general`
  channel
- break long lines
- remove the report commands as it's supposed to be used by the `check pr`
  command of the Nushell toolkit
- remove the `--dataframe` options as we do not have any feature here
  and especially not the *dataframe* one
- default to always using `--workspace` or `--all`
- remove the inline doc to `cargo clippy`: it can be accessed with
  `cargo help clippy`
- put the same *Clippy* rules as in the CI and on multiple lines
@amtoine
Copy link
Member

amtoine commented Jun 14, 2023

i've fixed the links to the Discord in 9123aa3 @AucaCoyan, hoping the commit body explains this well 😇

@AucaCoyan
Copy link
Contributor Author

i've fixed the links to the Discord in 9123aa3 @AucaCoyan, hoping the commit body explains this well 😇

All good to go! Thanks for the fixes!

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

All good to go! Thanks for the fixes!

cooool, thanks @AucaCoyan for the PR 😊

@amtoine amtoine merged commit 7770e3a into nushell:main Jun 14, 2023
4 checks passed
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.

None yet

3 participants