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

Run dartanalyzer directly #4

Closed
klavs opened this issue Feb 9, 2020 · 14 comments
Closed

Run dartanalyzer directly #4

klavs opened this issue Feb 9, 2020 · 14 comments
Labels
enhancement New feature or request

Comments

@klavs
Copy link

klavs commented Feb 9, 2020

Background
Pana is currently ignoring package's own analysis_options.yaml, instead using pedantic. pedantic:1.9.0 has introduced some controversial rules.

Proposal
In addition to running pana, which gives a good feel of how the package will look if published, also run dartanalyzer to give feedback on conformance to package's own rules even if a package not supposed to be published.

@axel-op
Copy link
Owner

axel-op commented Feb 9, 2020

The purpose of this action is to reflect the score that your package would have once it's published, thus I don't think I'll add a way to use the analysis_options file.

Why do you want this to be included in this action, as you can run dartanalyzer by yourself in your workflow?

@klavs
Copy link
Author

klavs commented Feb 9, 2020

The purpose of this action is to reflect the score that your package would have once it's published, thus I don't think I'll add a way to use the analysis_options file.

pub.dev currently is not using the latest version of pedantic , so this action is currently reporting a score which is not the same as you get when publishing to pub.

Why do you want this to be included in this action, as you can run dartanalyzer by yourself in your workflow?

I think, in practice, authors want to know the contributions are aligned to the package's own rules in addition to what pana suggests.

In case a package has a conflicting rule with what pana suggets, package authors may take action to align their rules to pana or purposely break them. But at least this action would highlight the discrepancies the package rules have.

If a package has it's own analysis_options.yaml, the annotations added by this action (taken from pana) are misguiding as they may suggest fixes that the author actually does not want.
(In my experiments with this action, you cannot even increase a severity of a rule, not to mention deviating from those rules)

On a practical note, you've got everything ready to publish annotations, so addition of direct usage of dartanalyzer would be quite easy.

I think the ability to run dartanalyzer directly from this action would allow package authors to cope with the confusion pub.dev, pana and pedantic have introduced.

@axel-op
Copy link
Owner

axel-op commented Feb 9, 2020 via email

@klavs
Copy link
Author

klavs commented Feb 9, 2020

To boil it down:

  • optionally use annotations from dartanalyzer
  • optionally skip pana annotations

@axel-op
Copy link
Owner

axel-op commented Feb 9, 2020

Here's how I would implement this: I would make it show a warning for each linting rule used by pana that is not in the analysis_options file (something like "These rules used by pana aren't in your file: ...").

  • optionally use annotations from dartanalyzer

I don't think I will do that because it seems to me that this is not the purpose of this action.

  • optionally skip pana annotations

I could eventually do that.

@axel-op
Copy link
Owner

axel-op commented Feb 9, 2020

  • optionally skip pana annotations

Btw, are you aware that you can filter annotations shown by their level? But I guess you just want to hide those related to linting?

@klavs
Copy link
Author

klavs commented Feb 9, 2020

That's unfortunate. I hoped I could use this action to improve my process. 😞

At the current state this action is quite useless. Take a look at this: https://github.com/gql-dart/gql/pull/44/checks?check_run_id=434318706

It shows 0 health:
image

Even though on pub it's 100:
image

Anyway, thanks for your great work! I guess I'll be forking it to add this feature to my workflow.

@axel-op
Copy link
Owner

axel-op commented Feb 9, 2020

@klavs If I said that this is not the purpose of this action, it's because you can already launch the dartanalyzer by yourself in your workflow (as you did as I saw), so this would be redundant.

About your package and the difference between the health score on the pub site and the one given by this action, after a little investigation I think that this is due to a bug of the pana package. I'm not entirely sure of that but I created an issue: dart-lang/pana#614.

@klavs
Copy link
Author

klavs commented Feb 9, 2020

If I said that this is not the purpose of this action, it's because you can already launch the dartanalyzer by yourself in your workflow (as you did as I saw), so this would be redundant.

No problem.

About your package and the difference between the health score on the pub site and the one given by this action, after a little investigation I think that this is due to a bug of the pana package. I'm not entirely sure of that but I created an issue: dart-lang/pana#614.

Nope, it's related to this:

Also: as a stopgap measure, we've limited the penalty for lints to 25 health points, and temporary reverted back to pedantic 1.8.0. These changes may not be live on pub.dev as of now, but will be soon.
dart-lang/pana#593 (comment)

InspectOptions.analysisOptionsUri to optionally control which pedantic version (or other package's) ruleset is used for analysis lints.
https://github.com/dart-lang/pana/blob/master/CHANGELOG.md#0134

And the general issue has been discussed here where I suggested to take analysis_options into account:
dart-lang/pana#545

@axel-op
Copy link
Owner

axel-op commented Feb 9, 2020

That's right! I knew I'd read something like that (I've even subscribed to this thread...).

However, when I tried to run pana on your repository on Windows, scores were both at 100. So I still think that my issue could be relevant.

@axel-op
Copy link
Owner

axel-op commented Feb 9, 2020

@klavs On a side note, I noticed that your workflow file has a lot of similar jobs that differ only on the package name. This is just a suggestion, but in case you don't already know that, there is a parameter called strategy: matrix that you can use in your workflow to execute the same job with different parameters.

@axel-op axel-op added the enhancement New feature or request label Feb 11, 2020
@axel-op
Copy link
Owner

axel-op commented Feb 29, 2020

Hey @klavs, I made a little update today that allows to use your own analysis options file. Tell me what you think about it!

@axel-op
Copy link
Owner

axel-op commented Feb 29, 2020

Well I drop it for now. For two reasons:

  • It is pretty confusing because the annotations of the analyzer do not reflect the pana summary.
  • My tests failed because, as you can see here, a command has to be run to update dependencies before running the analyzer, but it seems that the cache isn't preserved between the two commands, or something like that... I don't have time to investigate for now.

@axel-op
Copy link
Owner

axel-op commented May 7, 2020

I'm gonna close this issue, because I think that this action could approximately do what you want.

@axel-op axel-op closed this as completed May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants