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

[core] Replace TextFile::pathId #4425

Merged
merged 45 commits into from
May 29, 2023

Conversation

oowekyala
Copy link
Member

@oowekyala oowekyala commented Mar 19, 2023

Describe the PR

Replace the pathId and display name of TextFile with a FileId instance. This eliminates the confusion between both, because both were strings. It also allows renderers to pick more freely how to display the file name. For instance the Sarif renderer always requires URIs, however neither pathId or display name specify that they should be URIs. By default they use a FileNameRenderer (function FileId->String) which does the same relativization as we did before in the file collector. This also replaces RuleViolation::getFileName with RuleViolation::getFileId so that the id is given to renderers, and we don't hardcode a display method in the RuleViolation.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@oowekyala
Copy link
Member Author

Note that this actually reverts #4464, or rather fixes the problem in another way. I think the better parameter order is file name then source contents, as it was already in PMD 7 until #4464. However the file name parameter doesn't have type string anymore but FileId, so that removes the possible confusion.

@pmd-test
Copy link

pmd-test commented May 3, 2023

1 Message
📖 Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
Compared to master:
This changeset changes 6 violations,
introduces 1 new violations, 1 new errors and 0 new configuration errors,
removes 3 violations, 0 errors and 0 configuration errors.
Full report
Compared to master:
This changeset changes 8 violations,
introduces 1 new violations, 0 new errors and 0 new configuration errors,
removes 1 violations, 0 errors and 0 configuration errors.
Full report
Compared to master:
This changeset changes 10 violations,
introduces 2 new violations, 0 new errors and 0 new configuration errors,
removes 2 violations, 0 errors and 0 configuration errors.
Full report

Generated by 🚫 Danger

adangel added a commit to adangel/maven-pmd-plugin that referenced this pull request May 19, 2023
in RuleViolation.getFilename() -> RuleViolation.getFileId()->getFileName()

Refs pmd/pmd#4425
@oowekyala oowekyala marked this pull request as ready for review May 28, 2023 13:11
@adangel adangel added this to the 7.0.0 milestone May 29, 2023
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks!

I'll merge this now, as this affects many downstream libs/clients (maven-pmd-plugin, eclipse, potentially github-actions with sarif), so that we get out this breaking change early.

adangel added a commit to adangel/pmd that referenced this pull request May 29, 2023
adangel added a commit that referenced this pull request May 29, 2023
@adangel adangel merged commit fc13c32 into pmd:master May 29, 2023
3 checks passed
@oowekyala oowekyala deleted the pmd7-textfile-display-name branch May 29, 2023 19:29
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.

None yet

3 participants