Linter for drift

[dickermoshe]

Dunno what happened to the old PR. I think the VScode Github Plugin must've closed it. Anyway, here's it as a new PR

• reference latest schema in migration instead of step by step one
• createReturning with ignore mode
• offset without limit
• await all migrations
  - [ ] Manager report issues

[dickermoshe]

@simolus3

See how I setup the tests. Lmk if you have a better way of setting them up.

Also, Some of the async code is copied from the dart sdk, I don't know how this affects licensing.

[simolus3]

Some comments on the test infrastructure

[dickermoshe]

Right now warnings in manager generation are not reported in the resolver, so the linter can't use them

The warning I'm focusing on are duplicate reverse names e.g.

class User extends Table {
  late final group = integer().references(Group, #id)
  late final supergroup = integer().references(Group, #id)
}
class Group extends Table {
//   `userRefs` has 2 relations!!!
}

How would I go about this?

[dickermoshe]

I add a warning level to DriftAnalysisError to show orange squiggles instead of red for less severe errors

[simolus3]

Right now warnings in manager generation are not reported in the resolver, so the linter can't use them

Ideally, these warnings should be discovered in the analysis step instead of during code generation. I know that some of them are easier to report during codegen, is it a lot of work to port them over to something that could run in FileAnalyzer? The parts under if (element is BaseDriftAccessor) should have the same information available to them as the generator, especially in the end when all transitive tables have been added). I can also get started on this to set up the basic infrastructure for those warnings if that works for you.

[dickermoshe]

Yeah, could you please do that?
I tried myself, but after an hour I gave up.

[dickermoshe]

@simolus3 Any update on this.
Just need you to write some scaffolding where to do these analysis.

[dickermoshe]

In FileAnalyzer I don't have access to the base element to report the error

[dickermoshe]

Ping

[simolus3]

I had this commit laying around: b64d602#diff-d36a7b426417ff349d59d9768776e3b2d36ff88fcafe18d7f82c086db523c55d

I tried doing a rebase, but I appear to have messed up. I'm not sure why there are so many conflicts in the docs though...
I'll take another look at providing you with the correct definition spans though. You are essentially operating on a table model, right? Ideally we'd want to the same lints to work regardless of whether the table is defined in Dart or in a drift file, so I suppose you need a way to go from DriftColumn to the defining source span? Let me know if I'm overlooking something here, otherwise I can work on passing that information through the analyzer.

[dickermoshe]

drift_lint will only report IDE lints for .dart
Stated a discussion here to confirm that.

However, the custom_lint cli will still report errors so I guess it could be helpful

[dickermoshe]

At this point there is only a single lint which I need passed through all these layers.
Completely upending the entire analysis just for this would not be worth the effort

[github-actions]

🚀 Deployed on https://deploy-preview-3281--moor.netlify.app

[dickermoshe]

All done. I'll add docs once you give me the go ahead

[dickermoshe]

@simolus3 Yup, we def got a flaky test

[simolus3]

On the test, I blame the Dart update :D

[simolus3]

Looks good to me, thanks! I hope we don't need too much documentation for this because this should just work for users already using custom_lint.

I imagine a simple page in the CLI directory noting that the feature exists and what kind of errors are generally reported would be helpful. We should also link to that page from the analyze command docs.

[simolus3]

Not a huge fan of this, is there a way to tell custom_lint to use a different import for a package providing linters (my preferred library would be lib/integrations/custom_lint.dart).

But if that's not possible so be it, I can file an issue on custom_lint and we can leave this as-is until that gets resolved.

[dickermoshe]

??

[simolus3]

Sorry for the delay (again...) I can take a final look later today and merge this if it's ready from your side.

I'll probably do a patch release before merging this (there are some bugfixes waiting to be released), that way we can ship this in a minor version and I'll have more time to test this.

[simolus3]

I think we should have a small section somewhere mentioning this explicitly (it can also be linked at the bottom of this page).

I'm wary about mentioning the custom_lint dependency here. We're already suggesting users install a lot of dependencies.
IMO, we should:

1. Mention somewhere that if users already have custom_lint, drift will work seamlessly with that and provide its own lints.
2. Suggest (but less strongly than done here, essentially pretending custom_lint is a requirement) that users try custom_lint to get quicker feedback about potential issues related to drift.

I think the best way to do that would be to mention this on /Tools and then add a bullet point at the end of this page next to the others.

[dickermoshe]

Ill work on the docs.
Also seems CI is failing, I'll ping you when it's all ready