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

Added ability to baseline existing violations #873

Merged
merged 49 commits into from Apr 11, 2021

Conversation

frankdekker
Copy link
Contributor

@frankdekker frankdekker commented Apr 2, 2021

Type: Feature
Issue: Resolves #871
Breaking change: no

Added the ability to export existing violations to a (default) phpmd.baseline.xml file. When PHPMD notices this file all violations that exist within the baseline will not be added to the violations report. Violations are detected based on filename, method (if present), and the specific rule.

What's been changed?

  • Added Baseline classes to model, read and write baseline information.
  • Modified Command, CommandLineOptions and PHPMD to inject the baseline checks.
  • Added coverage for everything I've changed.
  • Updated README.rst and index.rst documentation with the new cli options. This page: https://phpmd.org/documentation/index.html

Some design decisions:

  • PHPMD by default will look for phpmd.baseline.xml next to the (first) ruleset given on the cli. Assumed default usage would be a phpmd.xml in the root of a project
  • A custom file location can be specified with the --baseline-file /path/to/file cli option.
  • When adding the cli option --generate-baseline the violations will be exported to the baseline xml instead of the default renderer. By adding this flag, the 2nd cli argument will be ignored.
  • The location of phpmd.baseline.xml will serve as the base path of the baselined files. All source files should be relative to this file. Eg, the expected location of phpmd.baseline.xml should be in the root of your project.

Examples

Export:
phpmd /path/to/source text phpmd.xml --generate-baseline
Writes baseline phpmd.baseline.xml next to phpmd.xml

Check:
phpmd /path/to/source text phpmd.xml
Tries to find phpmd.baseline.xml next to phpmd.xml

phpmd /path/to/source text phpmd.xml --baseline-file /path/to/file
Uses the given baseline filepath. All sources should still be relative to the baseline file.

Discussion A

I tried to stay close to the phpstan behaviour and what felt for me natural in the usages. It could be argued that the 2nd cli argument should be baseline. For example for export it could be:
phpmd /path/to/source baseline phpmd.xml

I've chosen the extra --generate-baseline flag, as the 2nd argument is for output for external purposes, while the baseline is for internal purposes. What are your thoughts?

Discussion B

I've chosen to determine the relative path of the violation source paths, to be relative to the baseline file. It felt natural for me to have all the files in the baseline file be relative to the file they are listed in. The downside is that with the -baseline-line cli option, you can't randomly choose a file location. Currently it has to be in a parent directory of the sources.

Other options would be to calculate the relative path to the phpmd.xml. This is kinda awkward as phpmd accepts multiple rulesets as 3rd argument. Would have to pick the location of the first ruleset

Third option would be to calculate the relative path based on the current working directory. This felt a little too magic for me, as the working directory could be outside the project.

Future

I read that psalm has an --update-baseline option, which will only remove entries from the baseline file, and won't add new ones. I think this will be nice feature for a future PR.

Added BaselineRenderer to write the baseline
Added ViolationBaseline
Added BaselineSetFactory
Copy link

@mirzazeyrek mirzazeyrek left a comment

Choose a reason for hiding this comment

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

Looks cool! Great work.

Copy link
Member

@kylekatarnls kylekatarnls left a comment

Choose a reason for hiding this comment

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

Tested and it works as expected.

@kylekatarnls kylekatarnls added this to the 2.10.0 milestone Apr 11, 2021
@kylekatarnls kylekatarnls merged commit 281a8da into phpmd:master Apr 11, 2021
@AJenbo
Copy link
Member

AJenbo commented Apr 11, 2021

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add a baseline for existing projects
4 participants