Skip to content
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: Add subjectPatternError option to set custom sub validation error #76

Merged
merged 9 commits into from
Jan 18, 2021

Conversation

derberg
Copy link
Contributor

@derberg derberg commented Jan 15, 2021

Resolves #74

@derberg derberg changed the title feat: add subjectPatternError option to set custom sub validation error feat: Add subjectPatternError option to set custom sub validation error Jan 15, 2021
Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work here @derberg!

The new feature seems to work as expected. I hope you found your way around in the code base 🙂.

Good idea with the interpolation of variables – I think this will improve the UX of the error message. Also thanks for adding tests!

I've left some comments with a few nits, but nothing major. Great work 👏.

README.md Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
src/SubjectPatternErrorParser.js Outdated Show resolved Hide resolved
src/SubjectPatternErrorParser.js Outdated Show resolved Hide resolved
src/validatePrTitle.test.js Outdated Show resolved Hide resolved
src/validatePrTitle.test.js Outdated Show resolved Hide resolved
@derberg
Copy link
Contributor Author

derberg commented Jan 18, 2021

@amannn hey, fixed applied, please have a look again

Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

I've unwrapped the tests for formatMessage from the describe block and added another test that verifies we can replace a single variable multiple times.

I'll wait for CI and then release this. Thanks again for your help!

src/formatMessage.test.js Outdated Show resolved Hide resolved
src/formatMessage.test.js Outdated Show resolved Hide resolved
@amannn amannn merged commit e617d81 into amannn:master Jan 18, 2021
@derberg derberg deleted the custom-pattern-error branch January 18, 2021 16:06
@derberg
Copy link
Contributor Author

derberg commented Jan 18, 2021

@amannn thanks man, it was a pleasure to contribute. Pure open source, good discussion on the issue, fast communication, great feedback on PR 🙏🏼

I hope you found your way around in the code base

I was only confused with node_modules being part of the code base and default npm install caused some headache. In my actions I just have an approach with compiling action to dist. Anyway, the code base was super good to get along with . We will use this action here for every repo in the organization -> https://github.com/asyncapi/

@amannn
Copy link
Owner

amannn commented Jan 18, 2021

That's great to hear! 🙂

I was only confused with node_modules being part of the code base

Right, when I created the first version of the action I was a bit confused that this is necessary. I saw that there's an option to compile a binary, but never got around to implementing that. Not sure how complex that really is. Do you have an example where you got that working?

@derberg
Copy link
Contributor Author

derberg commented Jan 18, 2021

yeah, that is simple, just

  1. add this https://github.com/derberg/global-workflows-support/blob/main/package.json#L38 dependency.
  2. add script like https://github.com/derberg/global-workflows-support/blob/main/package.json#L9
  3. and adjust action.yml https://github.com/derberg/global-workflows-support/blob/main/action.yml#L42
  4. remove node_modules from the upstream

normally that is it, but I was always forgetting to build the package 😅 so I have
5. this dependency https://github.com/derberg/global-workflows-support/blob/main/package.json#L43
6. add this script https://github.com/derberg/global-workflows-support/blob/main/package.json#L10
7. configure it https://github.com/derberg/global-workflows-support/blob/main/package.json#L16

I can configure that for you, not a problem, just tell me what you want, all or just the first part?

@amannn
Copy link
Owner

amannn commented Jan 19, 2021

Thanks for your hints! I took a stab at it in #77. It seems to work 🙂.

Btw. I noticed we forgot to test the positive case when a subjectPatternError is included and I noticed the action always failed when this option was configured. I've fixed this in #79.

@derberg
Copy link
Contributor Author

derberg commented Jan 19, 2021

I took a stab at it in #77

🥳

Btw. I noticed we forgot to test the positive case when a subjectPatternError is included and I noticed the action always failed when this option was configured. I've fixed this in #79.

ups, sorry for that

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.

Improvements for consideration
2 participants