Skip to content

Comments

chore: rewrite PR title linting workflow in py3#30

Closed
fa7ad wants to merge 5 commits intocloudscape-design:mainfrom
fa7ad:main
Closed

chore: rewrite PR title linting workflow in py3#30
fa7ad wants to merge 5 commits intocloudscape-design:mainfrom
fa7ad:main

Conversation

@fa7ad
Copy link

@fa7ad fa7ad commented Dec 2, 2022

Issue #, if available:
None
Description of changes:
This is a complete rewrite of the script in python, using as little regex as possible.

Why Python?

Github Actions natively support inline python script, and I'd argue its more readable than bash-ism like [[ and =~.

Why do you hate regex?

I like regex, but a long regex is very difficult to debug and opens us up to horrible possibilities like ReDoS. That said, feel free to use more regex features from python's re module to improve the script even more.

So this is just a rewrite? Why bother?

Because it's Friday, and there is an improvement (albeit small); you can now see why the linting failed.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@fa7ad fa7ad requested a review from a team as a code owner December 2, 2022 13:21
@fa7ad fa7ad requested review from avinashbot and michaeldowseza and removed request for a team and michaeldowseza December 2, 2022 13:21
fa7ad and others added 4 commits December 2, 2022 15:18
Co-authored-by: Avinash Dwarapu <dwaraa@amazon.com>
Co-authored-by: Avinash Dwarapu <dwaraa@amazon.com>
Co-authored-by: Avinash Dwarapu <dwaraa@amazon.com>
@fa7ad fa7ad requested a review from avinashbot December 2, 2022 14:20
@fa7ad fa7ad dismissed avinashbot’s stale review December 2, 2022 14:22

suggestion committed

@just-boris
Copy link
Member

We are a team of front-end engineers and our primary language is Javascript/Typescript. I got the point of python being built-in, but actions/setup-node also exists.

Copy link
Member

@avinashbot avinashbot left a comment

Choose a reason for hiding this comment

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

I definitely support merging this, regardless of whether it might be TypeScript/JavaScript in the future. It looks like the regex syntax was pretty bash-on-linux-only and not particularly portable, so this is an objective improvement. And a higher-level language is always nice for any future grammatical tricks.

I'll leave it to the team to decide whether we still want to consider redoing this in JavaScript in the future. But this PR's already here, and I like it. I'll leave it to you whether you think it's worth merging now or waiting until later.

@just-boris
Copy link
Member

Discussed as a team. All our infra uses Javascript, so this feature needs to be in JS too

@just-boris just-boris closed this Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants