Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 initial PR for Contributing & Coding Standard #920
Add initial PR for Contributing & Coding Standard #920
Changes from 27 commits
e8f0047
7a7b0fa
9ea997f
482bd08
cf7eb92
bb3077a
d2728d2
7094920
6a0f22c
483dc98
4bde4d7
fdedd55
2f095fc
3923184
a4d39c4
2ae6203
0aaea5c
784fc81
5e68a7e
d086288
2ddd79d
730abf5
b805a9e
b0d76cc
0e1f46b
4dc7a3d
acd26c4
d14a7d9
a822531
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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 think this sentence can be dropped.
It is in the similar context as the "why coding standards are useful".
Additionally, I don't know why I need to "please refer to this guide" when I am opening a PR. This doc are the guidelines to follow to create a PR. If a PR is not matching these guidelines, the feedback will be in this direction.
Not sure if adding a link to a PR to this doc helps. Rather than a checkbox in the PR template like "Followed and applied Coding standards" may be enough.
Why removing this paragraph?
It doesn't really add new information on what Coding standards itself. Following the rule of keeping it crisp and less is more.
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.
Yeah, like above, on second tougth it seems a bit overengineered.
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.
Without more context it is unclear what the meaning of this sentence is (yes I know it is the call to build.func)
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.
Its better explained int App.md. Maybe remove it here
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.
Yes after going through the rest of the files that would make a lot of sense. It is not important here (yet)
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 the idea to start a new script from a template.
Why not going all in and providing the CLI commands for this?
This way, folks can just copy / paste the commands and get going.
Additionally: Considering the CONTIRBUTING.md is the entry doc, this is the first time it is mentioned that we operate on a fork.
Maybe it is better to move this chapter to a
which starts with
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.
There has been a proposal by @quantumryuu for this already, have a look here: https://github.com/community-scripts/ProxmoxVE/tree/new_script_testing/.github/CONTRIBUTOR_GUIDE/dev-scripts
I will start testing, experementing with this when the hollidays are over.
I also would love to have something simillar to this for Appname.sh and AppName-install.sh
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.
About the AppName.sh/AppName-install.sh we could modify the scripts I proposed into asking things like "Author", "Source", "AppName" I guess.
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.
Here i am a bit lost. Where do I start now to build my own script? Here or with the template of
ct/AppName.sh
andinstall/AppName-install.sh
?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.
Whats the purpose of this chapter?
What goes in here? What does not go in here?
I think all relevant pages are linked above in the respective context. I am suggesting to drop this chapter.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
These do not seem to be best practices. These seem to be musts for contributions.
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.
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 don't understand this
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.