-
Notifications
You must be signed in to change notification settings - Fork 4
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 lintr continuous integration #96
Conversation
|
1852a2f
to
814de7c
Compare
1f8805a
to
0fcd994
Compare
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.
Thank you for adding {lintr} to this repo! It is a polite way to police a homogeneous writing aesthetic. I liked the homogenization of require/library and the isolation of argument object names in different lines.
Thanks to this PR, I also found the need to open #108 , so the nolint
will be displaced at some moment after emerging other PRs.
I asked some questions that probably require some changes before merging.
One additional question. About the warning in the second message of this PR, should we isolate the workflow and regular file edits in different PRs? |
It's fine in this specific case. This warning is here because there is a risk someone could steal private GitHub info from of our organization in such a PR. This security element is good in general. In this specific case, you don't have anything to worry about since I already have access to all our of organization settings. |
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 agree with the recent updates. Ready to merge! 🚀
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.
Sorry, I want to add the brief edit suggested in #108
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 appropriate, these suggested changes can help to solve #108
Co-authored-by: Andree Valle Campos <[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.
thank you for the latest updates! ready to merge 🚀
This is on hold until #93 is merged since it will already remove a bunch of lints.Please disregard the failing check. It is expected and innocuous.