-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
docs: Add renovate related best practices guide #15589
Conversation
First draft 😀 |
- cr changes
faf1fe6
to
e57ad26
Compare
docs/development/best-practices.md
Outdated
- Avoid underscore suffixes or prefixes, for example: `_prop`, use [whole words](https://google.github.io/styleguide/tsguide.html#properties-used-outside-of-class-lexical-scope) as | ||
suffix/prefix i.e `internalProp` |
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.
if we do plan to have this, do we want to permit some renovate related prefix for API scheme checking
like in here:
renovate/lib/modules/versioning/index.spec.ts
Lines 110 to 113 in b87b3b0
.filter( | |
(val) => !optionalFunctions.includes(val) && !val.startsWith('_') | |
) | |
.sort(); |
and here:
renovate/lib/modules/versioning/index.spec.ts
Lines 140 to 144 in b87b3b0
const schemeKeys = getAllPropertyNames(api) | |
.filter( | |
(val) => !optionalFunctions.includes(val) && !val.startsWith('_') | |
) | |
.sort(); |
as of now, underscore is used to indicate private class variables and methods (like _parse in genericVersioning) and the above
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.
We should probably be relaxed here, as we otherwise can't easily distinguish between private and public api 🤔
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.
so we leave the test as is.
should we keep this bullet point?
- cr changes
- cr changes
@Gabriel-Ladzaretti Why are you creating new commits, instead of applying my suggestions? Renovate contributing guide, apply maintainer provided review suggestions. |
No specific reason, i just prefer working from the IDE. |
Yes please do, it's easier to track for the reviewer as well as gives appropriate credit for the changes through GitHub |
Co-authored-by: HonkingGoose <[email protected]>
As a general rule, in this project we should always favor coding standards which can be enforced via linting. Are there any in this list which are already enforced, or which could be, e.g. via |
Co-authored-by: Rhys Arkins <[email protected]>
- cr changes
- docs update as per CR
Co-authored-by: HonkingGoose <[email protected]>
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.
some small annotations
Co-authored-by: Michael Kriese <[email protected]>
- docs update as per CR
docs/development/best-practices.md
Outdated
### Classes | ||
|
||
Use [Typescript getter setters (Accessors) when needed](https://google.github.io/styleguide/tsguide.html#properties-used-outside-of-class-lexical-scope). | ||
The getter must be a `pure function`. |
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.
what is pure
🤔
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.
should we add an explanation ? i.e
- are predictable: given the same input, always returns the same output.
- have no side effect
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.
Please add that explanation. 😉
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've added the explanation given in the provided wiki page.
hope its alright
Co-authored-by: Michael Kriese <[email protected]>
- docs update as per CR
- added pure function description
Head branch was pushed to by a user without write access
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.
otherwise LGTM
Co-authored-by: Michael Kriese <[email protected]>
🎉 This PR is included in version 32.66.5 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Changes
Nothing
Context
#15442
Documentation (please check one with an [x])
How I've tested my work (please tick one)
I have verified these changes via: