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

Ignore spaces after items in "Needs:" and "Tags:" lists #373

Merged
merged 11 commits into from
Feb 2, 2024

Conversation

sambishop
Copy link
Contributor

@sambishop sambishop commented Jan 29, 2024

Closes #364
This PR addresses issue 364: #364

I noticed that "Tags" lists behave the same way, so I updated them as well.

@sambishop
Copy link
Contributor Author

sambishop commented Jan 31, 2024

@kaklakariada, thank you for reviewing my PR. But it's been stuck for over a day now waiting for the SonarCloud analysis, and I suspect that it's never going to complete. Do you know how to get that step to run?

Also, I think this is a great project, so I thought I'd try addressing a few open issues as a way of showing my appreciation. So let me know if there's anything I should do to improve the quality of my PRs. I plan to open one PR per issue. Thanks.

@kaklakariada
Copy link
Contributor

Hi @sambishop ,
thank you very much for your PR, we really appreciate that! I don't know how to solve the issue with the Sonar analysis. The token for Sonar is not available in external PR so we run it only when the token is available:

    - name: Sonar analysis
      shell: bash
      if: ${{ env.DEFAULT_OS == matrix.os && env.DEFAULT_JAVA == matrix.java && env.SONAR_TOKEN != null }}
      run: |
        mvn --batch-mode org.sonarsource.scanner.maven:sonar-maven-plugin:sonar \
            -Dsonar.token=$SONAR_TOKEN
      env:
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}

I guess the pragmatic solution would be to merge the PR anyway after a review.

Another thing: I would like to add an entry to the changelog for your changes. I will do that directly in your PR if that's OK for you.

@kaklakariada
Copy link
Contributor

I added tests and found a bug with parsing the Tags: field and fixed it.
@redcatbear Please also have a look at my changes.

…de to 'return !(...);'. (It was flagged by Sonar as a code smell.)
@sambishop
Copy link
Contributor Author

I've never used Sonar on a public repo before. I've tried adding a Sonar token secret to my forked repo, to see if that allows it to run. And I've addressed the only two outstanding Sonar issues. Let's see what happens.

Thanks again for your help, @kaklakariada!

Copy link
Collaborator

@redcatbear redcatbear left a comment

Choose a reason for hiding this comment

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

Looks really good! Thanks for the PR.

Couple small improvements suggested.

@@ -1,5 +1,6 @@
# Changes

* [3.7.2](changes_3.7.2.md)
* [3.7.1](changes_3.7.1.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Last released version is 3.7.0. @kaklakariada, do you plan an intermediate release? If not, please move changes into 3.7.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

doc/changes/changes_3.7.2.md Outdated Show resolved Hide resolved
@@ -37,7 +37,7 @@ enum MdPattern
+ SpecificationItemId.ID_PATTERN
+ ").*?"),
ID("`?((?:" + SpecificationItemId.ID_PATTERN + ")|(?:" + SpecificationItemId.LEGACY_ID_PATTERN + "))`?.*"),
NEEDS_INT("Needs:\\s*(\\w+(?:,\\s*\\w+)*)"),
NEEDS_INT("Needs:(\\s*\\w+\\s*(?:,\\s*\\w+\\s*)*)"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Comment on lines +73 to +74
+ "\nNeeds: " + NEEDS_ARTIFACT_TYPE1 //
+ " , " + NEEDS_ARTIFACT_TYPE2 + " ";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch!

private List<SpecificationItem> runImporterOnText(final String text)
{
final BufferedReader reader = new BufferedReader(new StringReader(text));
final InputFile file = StreamInput.forReader(Paths.get(FILENAME), reader);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No intermediate file on the file system. I like it.

@kaklakariada kaklakariada merged commit 355f8b0 into itsallcode:main Feb 2, 2024
9 checks passed
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.

A space behind a Needs: specification item validates the requirement
3 participants