-
Notifications
You must be signed in to change notification settings - Fork 16
[GitHub Action] Automatically open a PR for accepted
guideline issues
#122
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
base: main
Are you sure you want to change the base?
Conversation
❌ Deploy Preview for scrc-coding-guidelines failed.
|
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.
Hey @x0rw thanks for taking this on. I haven't reviewed it in detail yet.
Noticed that you could use an existing label tho 👍
By the way, I see the build is failing -- is that intentional / expected?
I'd like to start reviewing once the build passes.
Hi @PLeVasseur. |
Also, let me know when to remove those json test issues because i picked them randomly from my repo. |
Hey @x0rw -- could you rebase? @darkwisebear recently update the spec lock file. |
e02a930
to
34a4740
Compare
test auto-pr template 2 test auto-pr template 3 test auto-pr template 4 test auto-pr template 5 test auto-pr template 6 test auto-pr template 7 test auto-pr template 8 test auto-pr template 9
Co-authored-by: Pete LeVasseur <[email protected]>
34a4740
to
2d60b33
Compare
Could you help me become less ignorant? 😅 Is it possible to enable anyone to try to auto-rebase? Is it this the Automatic Rebase action or something else? (that action looks kinda nice...) |
Hey @PLeVasseur 😄 |
@PLeVasseur, |
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.
Hey @x0rw -- sorry for the long delay 😅 I'm back from my "workation" in Japan and jumping back into reviews.
Bottom-line: I really like the concept and how you executed it.
Summary of feedback: As you can see from some of the comments I left, there's some refactoring that needs to be done to allow for easier maintenance in the future between both generate-guideline-templates.py
and auto-pr-helper.py
. Hope the rationale makes sense! 🙏
- name: Run Python script to generate guideline file | ||
run: | | ||
echo '${{ toJson(github.event.issue) }}' | python3 scripts/auto-pr-helper.py | ||
|
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.
It would be super nice here to try to build the coding guidelines in a step in-between here and then either:
- fail this action and report why as a comment on the issue (reading the build log and any errors generate to create the comment) or
- allow the PR to be opened and leave a comment on the PR as to why the build failed
I'm kind of partial to 1, but could be swayed. Could also be done in a follow-up PR. What do you think?
Tagging some others for their thoughts too
@vapdrs, @felix91gr, @plaindocs
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.
Did you have any thoughts on this @x0rw?
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.
Hey @PLeVasseur, I think commenting on the PR(2) is a better approach, as it opens the door for reviewer to make changes and fix minor problems.
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.
Seems fair to me. 👍
scripts/auto-pr-helper.py
Outdated
""" | ||
|
||
|
||
# taken from generate-guideline-templates.py |
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 know that refactoring this may be kind of un-fun, but I strongly think for maintenance reasons we should extract any shared code needed like this and then reference a function here and any place else like that in this file.
I'm not great at Python, but I'd imagine it's somehow possible to refactor generate_guideline_template()
from generate-guideline-templates.py
to pull this off.
e.g. pass in a dictionary to generate_guideline_template()
and if the relevant fields entries are filled extract and populate them.
Sorry, hope you understand 😅
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.
Thanks for flagging this, i did some refactoring around both.
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.
Thanks for understanding!
By the way -- are these lines that are generating IDs now not required to be here?
I see this work is now done in guideline_rst_template()
.
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 modified this nvim plugin i have, to hide all the comments and empty lines, and I always forget that it's enabled, i will disable it for now to pay attention to these issues.
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.
Ah, what I meant to ask is whether we still need these lines in guideline_template()
# generate random ids
guideline_id = generate_id("gui")
rationale_id = generate_id("rat")
non_compliant_example_id = generate_id("non_compl_ex")
compliant_example_id = generate_id("compl_ex")
Given that now this is done within generate_guideline_template()
.
Seemed like no, but wasn't sure.
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.
Ahh good catch, you're right -- we really need a linter to catch these earlier.
git config --global user.email "[email protected]" | ||
git config --global user.name "GitHub Action" | ||
|
||
# - name: Save issue JSON payload to file |
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.
Is this dead code needed?
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.
Bump!
add snapshot testing and their runner renamed generate-guideline-templates.py to generate_guideline_templates.py general refactor
Co-authored-by: Pete LeVasseur <[email protected]>
This workflow only triggers when the auto-pr script or the snapshot test directory changes
36406c5
to
b5c4829
Compare
About testing the auto-pr script, I went with snapshot testing approach, i compare the generated .rst template from issue json with the expected .rst result, |
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.
Thanks for taking on the refactoring @x0rw! I've suggested some bits here that I think could help. Could you take a look?
generate_guideline_templates.py
Outdated
def norm(value: str) -> str: | ||
return value.strip().lower() | ||
|
||
guideline_text = f""".. guideline:: {guideline_title.strip()} |
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.
Like the other comment I left, I think it could make it easier on the eyes if the whitespacing were as-if the contents of the .rst file were dropped in here.
Probably writing an example of what I mean will make this make more sense:
guideline_text = f"""\
.. guideline:: {guideline_title.strip()}
:id: {guideline_id}
...
:tags: {",".join(tags.strip().split())}
{amplification.strip()}
.. rationale::
:id: {rationale_id}
...
"""
I realize that this may imply that there's four spaces ahead of .. guideline:: {guideline_title.strip()}
written as I did above, but I've got to imagine there's some way to accomplish this that makes it formatted as if it were the contents of the .rst and also play nicely with surrounding parts of the program.
Some quick searches turned up the textwrap
library with its dedent()
function which
Remove any common leading whitespace from every line in text.
This can be used to make triple-quoted strings line up with the left edge of the display, while still presenting them in the source code in indented form.
So for example, from their linked docs above:
def test():
# end first line with \ to avoid the empty line!
s = '''\
hello
world
'''
print(repr(s)) # prints ' hello\n world\n '
print(repr(dedent(s))) # prints 'hello\n world\n'
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 it isn't great, I like your approach, although dedent can cause some issues with the code blocks indentations, it isn't that major.
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 see you've used dedent
now, so I'm curious to hear about this if it's worth sharing:
although dedent can cause some issues with the code blocks indentations, it isn't that major.
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.
dedent
basically strips common leading spaces to make code neat and tidy, the issue is that we actually add intentional indentations to the code blocks before saving them in the .rst file, if it's not set right(at the right point), it can remove those intentional indents, breaking formatting.
Co-authored-by: Pete LeVasseur <[email protected]>
Co-authored-by: Pete LeVasseur <[email protected]>
Co-authored-by: Pete LeVasseur <[email protected]>
Hey 👋.
This PR adds a workflow that automatically opens a PR for the accepted guidelines marked by 'accepted' label,
The script extracts the marked issue and parses it into a proper guideline rst file and appends this content into the appropriate chapter(eg. if the chapter is
concurrency
it will append the gl tosrc/coding-guidelines/concurreny.rst
, note: the chapters title in here should be directly mapped to the names in coding-guidelines/ directory).Tasks:
issue
from stdin and turn it into our guidelines' .rst formatgithub.event.issue
variableHow to test? :