Skip to content
This repository was archived by the owner on Jul 12, 2024. It is now read-only.

Conversation

@bsipocz
Copy link
Member

@bsipocz bsipocz commented Oct 2, 2017

This would add a note about rebasing to reset the clock.

cc @olebole

@bsipocz bsipocz requested a review from astrofrog October 2, 2017 13:28
@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.053% when pulling 7f508e2 on bsipocz:pr_close_message_update into f7578e9 on astropy:master.

@olebole
Copy link
Member

olebole commented Oct 2, 2017

Great, thank you very much!

@bsipocz bsipocz changed the title Updating to PR closing message Updating PR closing message Oct 2, 2017
*If you believe I commented on this pull request incorrectly, please report
this [here](https://github.com/astropy/astropy-bot/issues).*
"""
Copy link
Member

@astrofrog astrofrog Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you wrapped the text, you need to add, after the """:

""".replace(os.linesep, ' ').strip()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry I wasn't aware. Would you prefer them being unwrapped?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's easier to read like this, but we need to unwrap it programmatically

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or don't use """. Maybe something like

PULL_REQUESTS_CLOSE_WARNING = (
"line1\n"
"\n"
"line2\n")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer with the """ and the replace(...) stuff as then it's easy to re-wrap text automatically when adding more words.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fair point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, done. It had to be done with re as we want to keep some whitespace between the paragraphs.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 70.093% when pulling f6e328e on bsipocz:pr_close_message_update into f7578e9 on astropy:master.

@bsipocz
Copy link
Member Author

bsipocz commented Oct 19, 2017

@astrofrog - Can we merge this? Though probably it's not urgent again as the cron has already run just now.

*If you believe I commented on this pull request incorrectly, please report
this [here](https://github.com/astropy/astropy-bot/issues).*
""").strip()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to get rid of the wrapping here, so .replace(os.linesep, ' ') - also below

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait you did it with a regular expression, oops - any reason why not just use replace?

Copy link
Member Author

@bsipocz bsipocz Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it then replaces all new lines, and everything ends up in one line.

@astrofrog astrofrog merged commit e46245d into astropy:master Oct 19, 2017
astrofrog added a commit to OpenAstronomy/baldrick that referenced this pull request Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants