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: Allow enum options to be newline delimited only #205

Merged
merged 4 commits into from Oct 11, 2022

Conversation

jszwedko
Copy link
Contributor

@jszwedko jszwedko commented Oct 4, 2022

Fixes: #204

Signed-off-by: Jesse Szwedko jesse@szwedko.me

Fixes: amannn#204

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
@amannn
Copy link
Owner

amannn commented Oct 5, 2022

Thank you for your PR! I see your point and I'd be open to require newlines generally (not configurable).

Would you want to change this PR according to this and make sure the docs point the user towards that?

It will be a new major version then.

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
@jszwedko
Copy link
Contributor Author

jszwedko commented Oct 7, 2022

Thanks for the feedback @amannn ! I pushed another commit that removes the backwards compatibility and requires lists to be newline delimited. Let me know what you think.

@@ -1,4 +1,4 @@
const ENUM_SPLIT_REGEX = /[,\s]\s*/;
const ENUM_SPLIT_REGEX = /\n/;
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it makes sense since \r, by itself, is just a carriage return rather than a newline. For Windows file editing, it'd typically have \r\n which would still be split, and then subsequently trimmed, correctly. I'll push a change to add \r\n to the test.

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds fair, thank you for your feedback!

README.md Outdated Show resolved Hide resolved
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
@jszwedko
Copy link
Contributor Author

Thanks for the feedback @amannn ! I push a few more changes.

@jszwedko jszwedko requested a review from amannn October 11, 2022 13:53
@amannn amannn merged commit c906fe1 into amannn:main Oct 11, 2022
@github-actions
Copy link

🎉 This PR is included in version 5.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@ab-arao
Copy link

ab-arao commented Nov 2, 2023

This change did break our workflows because we have the following steps in github actions:

  1. determine valid scopes based on the file changed in the pull request
  2. pass those scopes to action-semantic-pull-request
  3. run action-semantic-pull-request using those scopes

We had been completing step 1 using a bash array and using $GITHUB_OUTPUT to pass the scopes as a space delimited string to step 3.

Below is how we are now implementing this in case anyone is in a similar situation.

jobs:
  validate-semantic-pull-request:
    runs-on: ubuntu-latest
    steps:
      - name: Check out code
        uses: actions/checkout@v4
      - name: Identify valid scopes
        id: scope-identifier
        run: |
          declare -a scopes=()

          // logic to figure out the valid scopes

          var=$( IFS=$'\n'; echo "${scopes[*]}" )

          echo 'scopes<<EOF' >> "$GITHUB_OUTPUT"
          echo "$var" >> "$GITHUB_OUTPUT"
          echo 'EOF' >> "$GITHUB_OUTPUT"
      - uses: amannn/action-semantic-pull-request@v5.3.0
        if: ${{ steps.scope-identifier.outputs.scopes }} != ""
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        with:
          scopes: |
            ${{ steps.scope-identifier.outputs.scopes }}
          requireScope: true

This solution was largely based on github/docs#21529.

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.

Scopes with whitespace aren't allowed
3 participants