-
Notifications
You must be signed in to change notification settings - Fork 5
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
[feat] Indent issue body when working with multi-issue file #24
Conversation
I should have setup pre-commit before submitting this. had to do an extra commit and learned the lesson haha |
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.
Hi @marceloFA, thanks for your PR!
I have a few comments on the code changes.
Also, while we're already poking about here, I figured we can refactor the _ask_for_open
function to use f-strings. Something like this:
def _ask_for_open(issue: plug.Issue, repo_name: str, trunc_len: int) -> bool:
indented_body = indent(issue.body[:trunc_len], INDENTATION_STR)
issue_description = (
f'\nProcessing issue "{issue.title}" for {repo_name}:\n{indented_body}'
)
plug.echo(issue_description)
return (
input(f'Open issue "{issue.title}" in repo {repo_name}? (y/n) ') == "y"
)
And then you need to work in that missing continuation indicator ([...]
) somewhere as well. What do you think?
If you're feeling extra productive, you could even extract the formatting of the issue description into a separate function, and add a couple of tests for the output.
I'm also a bit confused as to why the GitHub workflows haven't executed (or asked me to confirm a run, as is typically required for first-time contributors). Gonna have to look at that. |
Added some unit tests and did some refactor work. Let me know what you think! :) |
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.
Looks great! I just have some minor comments on the tests and then I think we're good 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.
Thanks @marceloFA, looks great!
Still have no idea why the tests aren't running, but I ran them locally and it's all good.
Fix #13
I opted to not implement terminal cleaning for each issue because of the scrolling up little problem. If you wish to have this feature (cleaning the terminal for each issue read), I can implement it, no problem :)
Please comment on anything that does not look good for you, I'm always totally open to suggestions and changes :D