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

bug(#368): unlint-non-existing-defect in WPA scope #369

Merged
merged 11 commits into from
Feb 28, 2025

Conversation

h1alexbel
Copy link
Contributor

In this pull I've introduced LtUnlintNonExistingDefectWpa to check +unlint meta for suppressions of non-existing defects in WPA scope.

ref #368
History:

@h1alexbel
Copy link
Contributor Author

@maxonfjvipon please check

this.lints.forEach(
wpl -> {
try {
final Collection<Defect> defects = wpl.defects(pkg);
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel can this line be move out of try catch scope? It seems that .defects does not throw IOException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxonfjvipon Lint#defects() throws IOException:

Collection<Defect> defects(T entity) throws IOException;

String.format(
"program/metas/meta[head='unlint' and tail='%s']/@line", unlint
)
)
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel there's something wrong with indentation again. I think it can be fixed by unlint -> xml.path(

this.lints.forEach(
wpl -> {
try {
final Collection<Defect> defects = wpl.defects(pkg);
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel it also seems that during the entire project linting we call Lint.defect a several times and on each particular lint. And every time these defects are recalculated. Maybe we should introduce some kind of cache or wrap these lints with cache decorator? Or at least add todo?

@h1alexbel
Copy link
Contributor Author

@maxonfjvipon updated. Take a look, please

String.format(
"program/metas/meta[head='unlint' and tail='%s']/@line", unlint
)
)
Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel something's still wrong with indentations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxonfjvipon otherwise qulice complains about cascade indentation

Copy link
Member

@maxonfjvipon maxonfjvipon Feb 27, 2025

Choose a reason for hiding this comment

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

@h1alexbel how about this:

.forEach(
    unlint -> xml.path(
        String.format(
            "program/metas/meta[head='unlint' and tail='%s']/@line", unlint
        )
    )
    .map(xnav -> xnav.text().get())
    .collect(Collectors.toList())
    .forEach(
        line -> defects.add(
            new Defect.Default(
                this.name(),
                Severity.WARNING,
                xml.element("program")
                    .attribute("name")
                    .text()
                    .orElse("unknown"),
                Integer.parseInt(line),
                String.format(
                    "Unlinting rule '%s' doesn't make sense, since there are no defects with it",
                    unlint
                )
            )
        )
    )
)

Copy link
Contributor Author

@h1alexbel h1alexbel Feb 27, 2025

Choose a reason for hiding this comment

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

@maxonfjvipon only this one is valid (dedent with xml.path() args):

.forEach(
    unlint -> xml.path(
        String.format(
            "program/metas/meta[head='unlint' and tail='%s']/@line", unlint
        )
        )
        .map(xnav -> xnav.text().get())
        .collect(Collectors.toList())
        .forEach(
            line -> defects.add(
                new Defect.Default(
                    this.name(),
                    Severity.WARNING,
                    xml.element("program")
                        .attribute("name")
                        .text()
                        .orElse("unknown"),
                    Integer.parseInt(line),
                    String.format(
                        "Unlinting rule '%s' doesn't make sense, since there are no defects with it",
                        unlint
                    )
                )
            )
        )
);

Copy link
Member

@maxonfjvipon maxonfjvipon Feb 27, 2025

Choose a reason for hiding this comment

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

@h1alexbel this part looks ugly:

  )
  )

We definitely should find another way. How about this:

.forEach(
    unlint -> xml.path(
            String.format(
                "program/metas/meta[head='unlint' and tail='%s']/@line", unlint
            )
        )
        .map(xnav -> xnav.text().get())

Copy link
Contributor Author

@h1alexbel h1alexbel Feb 27, 2025

Choose a reason for hiding this comment

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

@maxonfjvipon the best we can do is this, I believe (since we cannot inline xml.path due to too wide line):

.forEach(
    unlint -> xml.path(
        String.format(
            "program/metas/meta[head='unlint' and tail='%s']/@line", unlint
        )
        )
        .map(xnav -> xnav.text().get())
        .collect(Collectors.toList())
        .forEach(
            line -> defects.add(
                new Defect.Default(
                    this.name(),
                    Severity.WARNING,
                    xml.element("program")
                        .attribute("name")
                        .text()
                        .orElse("unknown"),
                    Integer.parseInt(line),
                    String.format(
                        "Unlinting rule '%s' doesn't make sense, since there are no defects with it",
                        unlint
                    )
                )
            )
        )
);

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@h1alexbel what about this?

.forEach(
    unlint -> xml
        .path(
            String.format()
        )
        .map(...)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxonfjvipon I think we did it. Big thanks!

@h1alexbel
Copy link
Contributor Author

@yegor256 take a look, please

Copy link
Member

@yegor256 yegor256 left a comment

Choose a reason for hiding this comment

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

@h1alexbel cool, thanks

@yegor256 yegor256 merged commit dbbcfde into objectionary:master Feb 28, 2025
18 checks passed
@h1alexbel h1alexbel deleted the 368 branch February 28, 2025 07:24
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.

3 participants