WIP linter for Effective Dart #31

GregorySech · 2019-10-09T16:54:18Z

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.

Excluding lint analysis for a file seems to be broken in CLI dart-lang/sdk#25551

fix avoid_shadowing_type_parameters warning I need confirmation if T comes from Box or if the shadowing is voluntary. Revert "fix avoid_shadowing_type_parameters warning" This reverts commit 19a64241c5dbee435a41bedf0344ce6d9996fb07. Reland "fix avoid_shadowing_type_parameters warning"

Merge dev changes

Must verify if it works

GregorySech · 2019-10-09T16:58:38Z

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

fix prefer_is_empty warning

GregorySech · 2019-10-09T17:47:50Z

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 · 2019-10-09T18:23:59Z

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 · 2019-10-09T18:24:40Z

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

vaind

LGTM so far

What warnings did you want to discuss?
Also, the docs for pedantic seem to indicate we should have an analysis_options.yaml file?

vaind · 2019-10-09T18:37:01Z

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 · 2019-10-09T18:47:42Z

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 · 2019-10-09T19:03:16Z

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 · 2019-10-09T19:22:20Z

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 · 2019-10-10T08:08:13Z

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 · 2019-10-10T15:47:24Z

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.

GregorySech · 2019-10-12T07:06:40Z

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 · 2019-10-12T10:19:53Z

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

vaind · 2019-10-12T14:26:53Z

FYI: raised an issue to avoid building the image on each pipeline: axel-op/dart-package-analyzer#1

GregorySech · 2019-10-13T08:54:56Z

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 nektos/act#74 .

GregorySech · 2019-10-17T12:45:00Z

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

vaind · 2019-10-17T12:54:17Z

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 · 2019-10-17T14:07:41Z

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 · 2019-10-17T14:13:31Z

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 · 2019-10-17T14:53:36Z

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 · 2019-10-17T15:01:06Z

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 · 2019-10-17T15:09:35Z

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 · 2019-10-17T15:11:20Z

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 · 2019-10-17T15:13:00Z

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 · 2019-10-17T15:34:16Z

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 · 2019-10-17T15:34:36Z

And thanks for the fixes! 🌮

GregorySech · 2019-10-17T15:36:47Z

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 · 2019-10-18T01:41:39Z

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:

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

vaind · 2019-10-21T10:16:36Z

I'll go with the new PR

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

Map common OBX_ERROR codes to exceptions Closes #31 See merge request objectbox/objectbox-dart!28

RandomDalek and others added 6 commits October 9, 2019 16:29

setup pedantic lint rules for Effective Dart

546f878

fix avoid_init_to_null warning

cd185dc

Excluding lint analysis for a file seems to be broken in CLI dart-lang/sdk#25551

Merge pull request #1 from objectbox/dev

b2e2e4e

Merge dev changes

fix avoid_types_as_parameter_names warning

2f55dd1

Must verify if it works

Merge branch 'dev' of github.com:GregorySech/objectbox-dart into dev

6e26d5a

GregorySech added 6 commits October 9, 2019 19:02

fix expected output after avoid_init_to_null change.

e175a59

fix unused_import warning

d58fc2c

fix curly_braces_in_flow_control_structures warning

bfdec2b

fix unnecessary_new warning

082acdd

fix prefer_contains warning

c06b4c1

fix prefer_is_empty warning

3638dc1

fix prefer_is_empty warning

vaind approved these changes Oct 9, 2019

View reviewed changes

GregorySech added 3 commits October 9, 2019 21:04

Merge branch 'dev' into dev

17dfbfe

setup dart_package_analyzer

187b1f2

Merge branch 'dev' of github.com:GregorySech/objectbox-dart into dev

ae0a55b

GregorySech added 4 commits October 10, 2019 16:31

move analysis actions to its own file.

c5cea8c

renaming analysis jobs

3fb312f

fix unawaited_futures warning

a62df9f

fix renaming analysis jobs.

0961a73

axel-op mentioned this pull request Oct 12, 2019

Use image on dockerhub instead of a Dockerfile axel-op/dart-package-analyzer#1

Closed

vaind mentioned this pull request Oct 14, 2019

Publish to pub.dev #33

Closed

GregorySech closed this Oct 21, 2019

GregorySech deleted the dev branch October 21, 2019 16:19

FredJul mentioned this pull request Sep 22, 2021

Flutter application crashes on iOS 15 #313

Closed

greenrobot-team added a commit that referenced this pull request Dec 13, 2022

Add StorageException, includes errorCode as property (#31)

6e9c606

greenrobot-team added a commit that referenced this pull request Dec 13, 2022

Throw DbFullException for error code OBX_ERROR_DB_FULL (#31)

2d2f1d3

greenrobot-team added a commit that referenced this pull request Dec 13, 2022

Add mappings for some common OBX_ERROR codes (#31)

9833f0e

greenrobot-team added a commit that referenced this pull request Dec 13, 2022

Do not map OBX_ERROR_STD codes to special exceptions (#31)

4c2cea5

greenrobot-team added a commit that referenced this pull request Dec 13, 2022

Replace RangeError with NumericOverflowException (#31)

2c3bc46

greenrobot-team added a commit that referenced this pull request Dec 13, 2022

Change findUnique to throw NonUniqueResultException (#31)

6a715be

greenrobot-team added a commit that referenced this pull request Dec 13, 2022

Merge branch '31-map-error-codes-to-exceptions' into 'main'

bbd388e

Map common OBX_ERROR codes to exceptions Closes #31 See merge request objectbox/objectbox-dart!28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP linter for Effective Dart #31

WIP linter for Effective Dart #31

GregorySech commented Oct 9, 2019

GregorySech commented Oct 9, 2019

GregorySech commented Oct 9, 2019

vaind commented Oct 9, 2019

GregorySech commented Oct 9, 2019

vaind left a comment

vaind commented Oct 9, 2019

GregorySech commented Oct 9, 2019

vaind commented Oct 9, 2019

GregorySech commented Oct 9, 2019

vaind commented Oct 10, 2019

GregorySech commented Oct 10, 2019

GregorySech commented Oct 12, 2019

GregorySech commented Oct 12, 2019

vaind commented Oct 12, 2019

GregorySech commented Oct 13, 2019

GregorySech commented Oct 17, 2019

vaind commented Oct 17, 2019

GregorySech commented Oct 17, 2019 •

edited

Loading

vaind commented Oct 17, 2019

GregorySech commented Oct 17, 2019 •

edited

Loading

vaind commented Oct 17, 2019

vaind commented Oct 17, 2019

GregorySech commented Oct 17, 2019

GregorySech commented Oct 17, 2019

vaind commented Oct 17, 2019

vaind commented Oct 17, 2019

GregorySech commented Oct 17, 2019

axel-op commented Oct 18, 2019 •

edited

Loading

vaind commented Oct 21, 2019

WIP linter for Effective Dart #31

WIP linter for Effective Dart #31

Conversation

GregorySech commented Oct 9, 2019

GregorySech commented Oct 9, 2019

GregorySech commented Oct 9, 2019

vaind commented Oct 9, 2019

GregorySech commented Oct 9, 2019

vaind left a comment

Choose a reason for hiding this comment

vaind commented Oct 9, 2019

GregorySech commented Oct 9, 2019

vaind commented Oct 9, 2019

GregorySech commented Oct 9, 2019

vaind commented Oct 10, 2019

GregorySech commented Oct 10, 2019

GregorySech commented Oct 12, 2019

GregorySech commented Oct 12, 2019

vaind commented Oct 12, 2019

GregorySech commented Oct 13, 2019

GregorySech commented Oct 17, 2019

vaind commented Oct 17, 2019

GregorySech commented Oct 17, 2019 • edited Loading

vaind commented Oct 17, 2019

GregorySech commented Oct 17, 2019 • edited Loading

vaind commented Oct 17, 2019

vaind commented Oct 17, 2019

GregorySech commented Oct 17, 2019

GregorySech commented Oct 17, 2019

vaind commented Oct 17, 2019

vaind commented Oct 17, 2019

GregorySech commented Oct 17, 2019

axel-op commented Oct 18, 2019 • edited Loading

vaind commented Oct 21, 2019

GregorySech commented Oct 17, 2019 •

edited

Loading

GregorySech commented Oct 17, 2019 •

edited

Loading

axel-op commented Oct 18, 2019 •

edited

Loading