Skip to content

GH-33687: [Dev] Fix commit message generation in merge script #33691

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.

Sign up for GitHub

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

Jump to bottom
Merged
pitrou merged 2 commits into from
Jan 16, 2023
Merged

GH-33687: [Dev] Fix commit message generation in merge script #33691

pitrou merged 2 commits into from
Jan 16, 2023

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Jan 16, 2023 •
edited by github-actions bot
Loading

@pitrou
apacheGH-33687: [Dev] Fix commit message generation in merge script
8f53bf8 
* Regex for removing HTML comments was pathologically slow because of greedy pattern matching
* Output of regex replacement was ignored (!)
* Collapse extraneous newlines in generated commit message
* Improve debugging output
@github-actions
Copy link

github-actions bot commented Jan 16, 2023

@pitrou
Copy link
Member Author

pitrou commented Jan 16, 2023

Hmm, I thought we had bumped the max line length but apparently not always?
Also, one of the lines flagged below wasn't initially modified by this PR...
https://github.com/apache/arrow/actions/runs/3929902654/jobs/6719260342#step:5:805

INFO:archery:Running Python linter (flake8)
/arrow/dev/merge_arrow_pr.py:578:80: E501 line too long (87 > 79 characters)
/arrow/dev/merge_arrow_pr.py:601:80: E501 line too long (86 > 79 characters)

@raulcd
Copy link
Member

raulcd commented Jan 16, 2023

Hmm, I thought we had bumped the max line length but apparently not always?

It seems the config wasn't being picked up. I have merged the PR that should really use the new max line length here: 2f8e2b2

raulcd
raulcd approved these changes Jan 16, 2023
View reviewed changes
Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

LGTM

@pitrou pitrou merged commit fa2f45d into apache:master Jan 16, 2023
@pitrou pitrou deleted the GH-33687-merge-script branch January 16, 2023 13:59
@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jan 16, 2023

Thanks for fixing this! (and sorry for too eagerly merging that previous PR without testing it myself ..)

@ursabot
Copy link

ursabot commented Jan 17, 2023

Benchmark runs are scheduled for baseline = 2f8e2b2 and contender = fa2f45d. fa2f45d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.24% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.51% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.47% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] fa2f45d3 ec2-t3-xlarge-us-east-2
[Finished] fa2f45d3 test-mac-arm
[Finished] fa2f45d3 ursa-i9-9960x
[Finished] fa2f45d3 ursa-thinkcentre-m75q
[Finished] 2f8e2b20 ec2-t3-xlarge-us-east-2
[Finished] 2f8e2b20 test-mac-arm
[Finished] 2f8e2b20 ursa-i9-9960x
[Finished] 2f8e2b20 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

raulcd pushed a commit that referenced this pull request Jan 18, 2023
@pitrou @raulcd
GH-33687: [Dev] Fix commit message generation in merge script (#33691)
461c17d 
* Regex for removing HTML comments was pathologically slow because of greedy pattern matching
* Output of regex replacement was ignored (!)
* Collapse extraneous newlines in generated commit message
* Improve debugging output

* Closes: #33687

Authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raulcd raulcd raulcd approved these changes

@jorisvandenbossche jorisvandenbossche Awaiting requested review from jorisvandenbossche

@assignUser assignUser Awaiting requested review from assignUser

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Dev] Merge script regex can be pathologically slow

4 participants

@pitrou @raulcd @jorisvandenbossche @ursabot