-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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 ✨
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 👍 |
I would love to! Tag me if you create it. |
I believe I addressed all the changes you requested, but feel free I you find some more or want to redo something! |
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
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! |
There was a problem hiding this 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 😊
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: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?