WIP linter for Effective Dart

[GregorySech]

I've started to integrate pedantic as the linter but I need some direction to fix some warnings. Also I need to actions to run once I'll add them to CI.

[GregorySech]

I've pushed from the wrong account, also I'll fix the test ASAP.

[GregorySech]

I'm almost done with the first fixes for the linter. I wanted to setup the action to automatically check for 0 lint errors before so we can see it fail. I've found this action https://github.com/marketplace/actions/dart-flutter-package-analyzer. It seems ok however this does package analysis too (it tells you the pub.dev quality score basically), should I try to integrate this or should I look for something else/try to make a custom action for lint only?

[vaind]

I've found this action https://github.com/marketplace/actions/dart-flutter-package-analyzer. It seems ok however this does package analysis too (it tells you the pub.dev quality score basically)

Looks very useful to me, let's give it a go

[GregorySech]

Ok, do you know if there is a way to test github actions locally?

[vaind]

Ok, do you know if there is a way to test github actions locally?

I'm not aware of that but it's no problem if you commit your changes. I expect GitHub to execute your "actions" changes here in the PR. In that case, you can wait for the actions to finish to see the changes on the 'checks' tab in the PR and reiterate using git reset --soft HEAD~1 and git push --force to still have a clean history with a single commit setting up actions.

[GregorySech]

LGTM so far

What warnings did you want to discuss?

So:

• in lib/src/generator.dart:121 we have a unawaited_futures that is not yet fixed. Basically we are doing asynchronous work and keep going without waiting for it to end. I was not sure about putting an await as it is a conceptual change.
• in lib/src/box.dart:99 I have assumed that the generic for the function is shadowing the T generic of the Box class so I renamed it to Q. I choose to consider it a "involuntary" shadowing because having the function return Box's T would break type checking for _runInTransaction current calls. I wanted to make sure this was the case.

Also, the docs for pedantic seem to indicate we should have an analysis_options.yaml file?

We have but we just import the pedantic rules right now. If we need to change some rules we can do so there.

About the action I've edited the workflow and have a commit ready however it seems like we need a secret so that results can be posted as comments. This line basically: githubToken: ${{ secrets.GITHUB_TOKEN }}. I cannot set that secret up and wanted to know if GITHUB_TOKEN is fine as a key or if a different key is better. Maybe PANA_GITHUB_TOKEN so you know that it's about package analysis?

[vaind]

LGTM so far What warnings did you want to discuss?

So:

• in lib/src/generator.dart:121 we have a unawaited_futures that is not yet fixed. Basically we are doing asynchronous work and keep going without waiting for it to end. I was not sure about putting an await as it is a conceptual change. I'm pretty sure that was unintended - feel free to change to a synchronous call/await

• in lib/src/box.dart:99 I have assumed that the generic for the function is shadowing the T generic of the Box class so I renamed it to Q. I choose to consider it a "involuntary" shadowing because having the function return Box's T would break type checking for _runInTransaction current calls. I wanted to make sure this was the case.

runInTransaction() wasn't even supposed to be in Box, I've just moved it, pls merge/rebase dev branch

Also, the docs for pedantic seem to indicate we should have an analysis_options.yaml file?

We have but we just import the pedantic rules right now. If we need to change some rules we can do so there.

Ah, it was the very first file... my bad

About the action I've edited the workflow and have a commit ready however it seems like we need a secret so that results can be posted as comments. This line basically: githubToken: ${{ secrets.GITHUB_TOKEN }}. I cannot set that secret up and wanted to know if GITHUB_TOKEN is fine as a key or if a different key is better. Maybe PANA_GITHUB_TOKEN so you know that it's about package analysis?

Would have to ask @greenrobot to set up a token for this repo. In the meantime, is there an option to just fail (actions fail if a command returns a non-zero return code)

[GregorySech]

It has got really slow... At least it doesn't fail without the key, it simply doesn't post a comment. We still have the unawaited_futures warning to handle somehow. Also pana took a lot of points, should we fix them here or in a different PR?

[vaind]

Yes it is rather slow, I think it would be preferable to have it a separate step, actually If I understand GitHub Actions correctly, it can be a separate file, e.g. analysis.yaml next to the existing dart.yaml

We still have the unawaited_futures warning to handle somehow. Does using await right with the call help? I think it wasn't intentional to have it asynchronous.

Also pana took a lot of points, should we fix them here or in a different PR? I'd prefer finishing and merging this before going ahead with the other reports (unless they're quick and obvious fixes)

[GregorySech]

Using different jobs seems to have no impact. I'm just wondering if it's linked to the missing secret. Maybe it's some timeout in the action. Just a supposition tho, I'll look into the action's code later.

On another note I was looking at the pana reports and there seems to be a problem with dartfmt's line length, it seems that it's run on 80c lines. On pana's cli there is an argument to consider 120 lines but I'm not sure that pub.dev uploaders can configure the website analysis.

[vaind]

Using different jobs seems to have no impact. I'm just wondering if it's linked to the missing secret. Maybe it's some timeout in the action. Just a supposition tho, I'll look into the action's code later.

Maybe it's something like that though I suspect it may be rather caused by a large docker image pull... I'd have to observe actions log while they're running. Can't see a way to launch it manually as in GitLab though... In any case, two minutes would be still OK-ish for now.

On another note I was looking at the pana reports and there seems to be a problem with dartfmt's line length, it seems that it's run on 80c lines. On pana's cli there is an argument to consider 120 lines but I'm not sure that pub.dev uploaders can configure the website analysis.

I don't think I can bring myself to using 80c lines (yet)... I'm pretty dumbfounded by that decision by the dart team... Are we going to start programming on our phones now?

[GregorySech]

Makes sense I'll look into it after work.

About the 80c I've asked around on Gitter but I've got no clear answer.

[GregorySech]

Ok I really have no idea why pana is failing on the generator. I'll look into it later in the morning or in the afternoon.

[GregorySech]

I've found this https://github.com/nektos/act and I'll try it to test the actions locally

[vaind]

FYI: raised an issue to avoid building the image on each pipeline: https://github.com/axel-op/dart_package_analyzer/issues/1

[GregorySech]

I've found out why the analysis fails. The problem is that the objectbox_model_generator package is located inside the bin directory which is by all means a source directory of the objectbox package.

Moving the package is needed, inspiration could be taken from https://github.com/google/built_value.dart

I had no luck in using act because of https://github.com/nektos/act/issues/74 .

[GregorySech]

I'll merge the changes this afternoon, should I move the two packages?

[vaind]

I'll merge the changes this afternoon, should I move the two packages?

Where would they move? I couldn't derive that from a look at built_value package

[GregorySech]

I'll merge the changes this afternoon, should I move the two packages?

Where would they move? I couldn't derive that from a look at built_value package

I'd put them in two different directories. built_value and built_value_generator are not nested in one another, they are at the same level in the repo FS.

Basically:

objectbox/
    <objectbox package files>
objectbox_model_generator/
    <objectbox_model_generator package files>
<repo README.md and other non package specific files>

edit: grammar

[vaind]

Right, so they'd be basically two separate projects. Yeah, I guess it's fine. Could you pls paste the analysis report that has lead you to this?

[GregorySech]

This is one of the pieces that got my attention in the report, the rest is still available in CI: https://github.com/objectbox/objectbox-dart/pull/31/checks?check_run_id=257476254

2019-10-12T06:45:47.8815403Z   "bin/objectbox_model_generator/lib/builder.dart": {
2019-10-12T06:45:47.8815770Z    "uri": "asset:objectbox/bin/objectbox_model_generator/lib/builder.dart",
2019-10-12T06:45:47.8816072Z    "size": 260,
2019-10-12T06:45:47.8816701Z    "isFormatted": false,
2019-10-12T06:45:47.8817008Z    "codeProblems": [
2019-10-12T06:45:47.8817343Z     {
2019-10-12T06:45:47.8817539Z      "severity": "ERROR",
2019-10-12T06:45:47.8817900Z      "errorType": "COMPILE_TIME_ERROR",
2019-10-12T06:45:47.8818172Z      "errorCode": "URI_DOES_NOT_EXIST",
2019-10-12T06:45:47.8818731Z      "file": "bin/objectbox_model_generator/lib/builder.dart",
2019-10-12T06:45:47.8819075Z      "line": 1,
2019-10-12T06:45:47.8821697Z      "col": 8,
2019-10-12T06:45:47.8822372Z      "description": "Target of URI doesn't exist: 'package:build/build.dart'."
2019-10-12T06:45:47.8822545Z     },
2019-10-12T06:45:47.8822625Z     {
2019-10-12T06:45:47.8822753Z      "severity": "ERROR",
2019-10-12T06:45:47.8822886Z      "errorType": "COMPILE_TIME_ERROR",
2019-10-12T06:45:47.8823020Z      "errorCode": "URI_DOES_NOT_EXIST",
2019-10-12T06:45:47.8823160Z      "file": "bin/objectbox_model_generator/lib/builder.dart",
2019-10-12T06:45:47.8823297Z      "line": 2,
2019-10-12T06:45:47.8823422Z      "col": 8,
2019-10-12T06:45:47.8823812Z      "description": "Target of URI doesn't exist: 'package:source_gen/source_gen.dart'."
2019-10-12T06:45:47.8823965Z     },
2019-10-12T06:45:47.8824605Z     {
2019-10-12T06:45:47.8825015Z      "severity": "ERROR",
2019-10-12T06:45:47.8825260Z      "errorType": "COMPILE_TIME_ERROR",
2019-10-12T06:45:47.8825500Z      "errorCode": "URI_DOES_NOT_EXIST",
2019-10-12T06:45:47.8825742Z      "file": "bin/objectbox_model_generator/lib/builder.dart",
2019-10-12T06:45:47.8825983Z      "line": 3,
2019-10-12T06:45:47.8826584Z      "col": 8,
2019-10-12T06:45:47.8827346Z      "description": "Target of URI doesn't exist: 'package:objectbox_model_generator/src/generator.dart'."
2019-10-12T06:45:47.8827531Z     },
2019-10-12T06:45:47.8827656Z     {
2019-10-12T06:45:47.8827779Z      "severity": "ERROR",
2019-10-12T06:45:47.8827925Z      "errorType": "STATIC_WARNING",
2019-10-12T06:45:47.8828057Z      "errorCode": "UNDEFINED_CLASS",
2019-10-12T06:45:47.8828194Z      "file": "bin/objectbox_model_generator/lib/builder.dart",
2019-10-12T06:45:47.8828289Z      "line": 5,
2019-10-12T06:45:47.8828431Z      "col": 1,
2019-10-12T06:45:47.8828774Z      "description": "Undefined class 'Builder'."
2019-10-12T06:45:47.8829034Z     },
2019-10-12T06:45:47.8829317Z     {
2019-10-12T06:45:47.8829469Z      "severity": "ERROR",
2019-10-12T06:45:47.8829604Z      "errorType": "STATIC_WARNING",
2019-10-12T06:45:47.8829753Z      "errorCode": "UNDEFINED_CLASS",
2019-10-12T06:45:47.8829851Z      "file": "bin/objectbox_model_generator/lib/builder.dart",
2019-10-12T06:45:47.8830013Z      "line": 5,
2019-10-12T06:45:47.8830338Z      "col": 31,
2019-10-12T06:45:47.8830755Z      "description": "Undefined class 'BuilderOptions'."
2019-10-12T06:45:47.8830905Z     },
2019-10-12T06:45:47.8831031Z     {
2019-10-12T06:45:47.8831157Z      "severity": "ERROR",
2019-10-12T06:45:47.8831291Z      "errorType": "STATIC_TYPE_WARNING",
2019-10-12T06:45:47.8831429Z      "errorCode": "UNDEFINED_FUNCTION",
2019-10-12T06:45:47.8831526Z      "file": "bin/objectbox_model_generator/lib/builder.dart",
2019-10-12T06:45:47.8831664Z      "line": 5,
2019-10-12T06:45:47.8831787Z      "col": 58,
2019-10-12T06:45:47.8832150Z      "description": "The function 'SharedPartBuilder' isn't defined."
2019-10-12T06:45:47.8832412Z     },
2019-10-12T06:45:47.8832540Z     {
2019-10-12T06:45:47.8832663Z      "severity": "ERROR",
2019-10-12T06:45:47.8832790Z      "errorType": "STATIC_TYPE_WARNING",
2019-10-12T06:45:47.8832877Z      "errorCode": "UNDEFINED_FUNCTION",
2019-10-12T06:45:47.8833010Z      "file": "bin/objectbox_model_generator/lib/builder.dart",
2019-10-12T06:45:47.8833152Z      "line": 5,
2019-10-12T06:45:47.8833271Z      "col": 77,
2019-10-12T06:45:47.8833611Z      "description": "The function 'EntityGenerator' isn't defined."
2019-10-12T06:45:47.8833752Z     }
2019-10-12T06:45:47.8834978Z    ],
2019-10-12T06:45:47.8835135Z    "directLibs": [],
2019-10-12T06:45:47.8835216Z    "platform": {
2019-10-12T06:45:47.8835338Z     "components": [],
2019-10-12T06:45:47.8835461Z     "uses": {
2019-10-12T06:45:47.8835816Z      "flutter": "allowed",
2019-10-12T06:45:47.8835945Z      "web": "allowed",
2019-10-12T06:45:47.8836075Z      "other": "allowed"
2019-10-12T06:45:47.8836190Z     }
2019-10-12T06:45:47.8836298Z    }
2019-10-12T06:45:47.8836375Z   },

edit: better phrasing

[vaind]

Ah, I didn't realize there are reports, let alone "errors" when the action passes. I'd have expected the CI checks to fail, like shown in the screenshot here for example: https://github.com/marketplace/actions/run-golangci-lint-with-reviewdog

Is this something we can configure? Maybe a question for @axel-op?

[vaind]

To get back to a rename, let's postpone that. I'm now going to try to cherry-pick formatting related commits so this PR doesn't get out of hand (too diverged from dev and other branches)

[GregorySech]

Ah, I didn't realize there are reports, let alone "errors" when the action passes. I'd have expected the CI checks to fail, like shown in the screenshot here for example: https://github.com/marketplace/actions/run-golangci-lint-with-reviewdog

Is this something we can configure? Maybe a question for @axel-op?

I was a little confused the first time however I'm not sure which would be a better semantic. Maybe some parameterized failing strategies would be globally appreciated.

Something like "fail_on_warning: bool", "fail_on_error: bool", "fail_lower_when: double" (last one being the score).

[GregorySech]

To get back to a rename, let's postpone that. I'm now going to try to cherry-pick formatting related commits so this PR doesn't get out of hand (too diverged from dev and other branches)

Maybe we could revert the CI integration and do a different PR for that. Linter should be fine now

[vaind]

To get back to a rename, let's postpone that. I'm now going to try to cherry-pick formatting related commits so this PR doesn't get out of hand (too diverged from dev and other branches)

Maybe we could revert the CI integration and do a different PR for that. Linter should be fine now

I've just pushed your linter related commits to dev. You can either create a new PR with the CI or rebase this one, dropping the old commits - whichever works for you better

[vaind]

And thanks for the fixes! :taco:

[GregorySech]

To get back to a rename, let's postpone that. I'm now going to try to cherry-pick formatting related commits so this PR doesn't get out of hand (too diverged from dev and other branches)

Maybe we could revert the CI integration and do a different PR for that. Linter should be fine now

I've just pushed your linter related commits to dev. You can either create a new PR with the CI or rebase this one, dropping the old commits - whichever works for you better

I'll go with the new PR

[axel-op]

Ah, I didn't realize there are reports, let alone "errors" when the action passes. I'd have expected the CI checks to fail, like shown in the screenshot here for example: https://github.com/marketplace/actions/run-golangci-lint-with-reviewdog Is this something we can configure? Maybe a question for @axel-op?

Again, thanks for the suggestion. Your comment made me read the documentation more carefully and I found out that GitHub Actions can also use, as Apps, the 'Checks' API that gives the possibility to post annotations on files.

I implemented this during the last hours, and there are two changes:

1. I replaced the minScoreToComment parameter with minAnnotationLevel
2. The action will now fail in case there is an error-level annotation

[vaind]

I'll go with the new PR

Thanks, please don't forget to close this one when you don't need it anymore.