axel-op / dart-package-analyzer

GitHub Action that uses the Dart Package Analyzer to compute the Pub score of Dart/Flutter packages
MIT License
52 stars 9 forks source link

Run dartanalyzer directly #4

Closed klavs closed 4 years ago

klavs commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

So to be clear, you want this action:

klavs commented 4 years ago

To boil it down:

axel-op commented 4 years ago

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 commented 4 years ago
  • 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 commented 4 years ago

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 commented 4 years ago

@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: https://github.com/dart-lang/pana/pull/614.

klavs commented 4 years ago

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. https://github.com/dart-lang/pana/issues/593#issuecomment-574585526

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: https://github.com/dart-lang/pana/issues/545

axel-op commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

Well I drop it for now. For two reasons:

axel-op commented 4 years ago

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