feat: Use linters from regular dependencies #276

dickermoshe · 2024-09-18T12:17:56Z

As of now custom_lint doesn't look for linters installed as regular dependencies. This would change that.

Semi-Fix for broken CustomLintWorkspace resolvePluginHost runs offline if possible test too

#275

@rrousselGit What sort of tests should be written for this (if any)

docs-page · 2024-09-18T12:18:00Z

To view this pull requests documentation preview, visit the following URL:

docs.custom-lint.dev/~276

Documentation is deployed and generated using docs.page.

vercel · 2024-09-18T12:18:00Z

@dickermoshe is attempting to deploy a commit to the Invertase Team on Vercel.

A member of the Team first needs to authorize it.

CLAassistant · 2024-09-18T12:18:07Z

All committers have signed the CLA.

rrousselGit · 2024-09-18T12:21:46Z

We'd want a test where a package depends on a lint using dependencies: and verify that the lints are correctly run

There are similar tests in the packages/custom_lint/test folder

dickermoshe · 2024-09-18T20:01:54Z

Seems to be a bug in how the workspace pubspec.yaml file is written.
I hope to get that working soon...

rrousselGit · 2024-09-18T20:09:36Z

It may not be your fault. The master CI is failing AFAIK

dickermoshe · 2024-09-19T05:17:05Z

@rrousselGit
I spent WAY too much time on this today.

I've written 2 new tests for this PR. And everything passes.

Except..
On main, there is 1 test that always fails (CustomLintWorkspace resolvePluginHost runs offline if possible) and another that is only fails sometimes (hot-reload Supports starting custom_lint twice in watch mode at once).

This is not my fault as you stated. Seems that there is some broken code which is already on invertase/dart_custom_lint:main

Logs

mick@MD-LAPTOP-1:~/dart_custom_lint/packages/custom_lint$ dart test
Building package executable... (2.0s)
Built test:test.
00:08 +18 -1: test/src/workspace_test.dart: CustomLintWorkspace resolvePluginHost runs offline if possible [E]                         
  Exception: Failed to run "pub get" in the client project:
  Resolving dependencies...
  
  pubspec.yaml has no lower-bound SDK constraint.
  You should edit pubspec.yaml to contain an SDK constraint:
  
  environment:
    sdk: '^3.5.0'
  
  See https://dart.dev/go/sdk-constraint
  
  package:custom_lint/src/workspace.dart 831:7  CustomLintWorkspace.runPubGet
  ===== asynchronous gap ===========================
  package:custom_lint/src/workspace.dart 815:5  CustomLintWorkspace.resolvePluginHost
  ===== asynchronous gap ===========================
  test/src/workspace_test.dart 891:9            main.<fn>.<fn>.<fn>
  

To run this test again: /home/mick/development/flutter/bin/cache/dart-sdk/bin/dart test test/src/workspace_test.dart -p vm --plain-name 'CustomLintWorkspace resolvePluginHost runs offline if possible'
01:36 +105 -2: test/server_test.dart: hot-reload Supports starting custom_lint twice in watch mode at once [E]                         
  Bad state: Cannot add event after closing
  dart:async                                                          _StreamController.add
  test/mock_fs.dart 100:17                                            _StdoutOverride.writeln
  package:custom_lint/src/plugin_delegate.dart 232:12                 CommandCustomLintDelegate.serverError
  package:custom_lint/src/v2/custom_lint_analyzer_plugin.dart 233:18  CustomLintServer.handleUncaughtError.<fn>
  ===== asynchronous gap ===========================
  package:custom_lint/src/async_operation.dart 22:14                  PendingOperation.run
  

To run this test again: /home/mick/development/flutter/bin/cache/dart-sdk/bin/dart test test/server_test.dart -p vm --plain-name 'hot-reload Supports starting custom_lint twice in watch mode at once'
  Bad state: Cannot add event after closing
  dart:async                                                          _StreamController.add
  test/mock_fs.dart 100:17                                            _StdoutOverride.writeln
  package:custom_lint/src/plugin_delegate.dart 232:12                 CommandCustomLintDelegate.serverError
  package:custom_lint/src/v2/custom_lint_analyzer_plugin.dart 233:18  CustomLintServer.handleUncaughtError.<fn>
  ===== asynchronous gap ===========================
  package:custom_lint/src/async_operation.dart 22:14                  PendingOperation.run
  
  Bad state: Cannot add event after closing
  dart:async                                                          _StreamController.add
  test/mock_fs.dart 100:17                                            _StdoutOverride.writeln
  package:custom_lint/src/plugin_delegate.dart 232:12                 CommandCustomLintDelegate.serverError
  package:custom_lint/src/v2/custom_lint_analyzer_plugin.dart 233:18  CustomLintServer.handleUncaughtError.<fn>
  ===== asynchronous gap ===========================
  package:custom_lint/src/async_operation.dart 22:14                  PendingOperation.run
  
04:48 +131 -2: Some tests failed.                                                                                                      

Consider enabling the flag chain-stack-traces to receive more detailed exceptions.
For example, 'dart test --chain-stack-traces'.

Either way, this PR is done and ready for review!

dickermoshe · 2024-09-19T13:59:05Z

@rrousselGit

When do you think you guys will be releasing a new version of custom_lint once this PR is merged?

dickermoshe · 2024-09-19T23:56:15Z

I really find this package useful so I decided to spend some time trying to fix this existing bug.

I managed to fixed the Dart SDK error in the CustomLintWorkspace resolvePluginHost runs offline if possible test.
But it turns out there is another bug in the test. Yay?!

I'm not sure what this test is trying to do , so I can't help fix it.
I was under the impression that dart pub get needs an internet connection, so I'm not sure what the purpose of this test is, ot how to proceed on fixing it.

Just to be clear, this is your bug, not mine. So IMHO, this shouldn't hold up the PR

rrousselGit · 2024-10-02T01:24:55Z

Hello!
Do you think you could sync with the master branch?

Then, if the CI is passing I'll evaluate whether this can be merged :)

dickermoshe · 2024-10-02T03:44:09Z

I thought you forgot about me 😢

I synced with main and tests pass on my end.

Lmk what changes you would want

packages/custom_lint/lib/src/workspace.dart

rrousselGit · 2024-10-02T09:52:38Z

packages/custom_lint/lib/src/workspace.dart

+      // Determine if this is a dev_dependency or a dependency.
+      // Note: Dependencies can't be in both `dependencies` and `dev_dependencies`.
+      // So these isNonEmpty checks mutually exclusive.
+      final bool isDevDependency;


We should preserve the source of a dependency.
If it's from dependencies, we write it as dependencies. If it is a dev_dependency, we write it as so.
And the constraints should match.

But what if it's in both?

e.g.

. ├── b │ └── pubspec.yaml # drift_dev in dev_dependencies ├── c │ └── pubspec.yaml # drift_dev in dependencies └── a └── pubspec.yaml # Root Package , depends on "b" and "c"

Project A depends on package B and C.
If drift_devis a dev_dependencies in package B and a regular dependency in C, how should it be written?

I spent a while trying to understand what this code is doing. If my assumptions are wrong please enlighten me

Actually, I think we should always be writing plugins to dependencies: , not dev_dependencies:

Because plugins use them directly rather than through command. If the linter was running against its own source, we'd probably see a warning that plugins should be in "dependencies" instead of dev_dependencies.

It works regardless because Dart sometimes doesn't care. But the current code is probably not right.

(we want to preserve dependency_overrides though, those shouldn't change)

Because plugins use them directly rather than through command. If the linter was running against its own source, we'd probably see a warning that plugins should be in "dependencies" instead of dev_dependencies.

Good point.

I've updated the code to combine dev_dependencies and dependencies and to always write them as regular dependencies in the generated pubspec.

I've updated the tests and wrote a new test for dealing with conflicting dev and regular dependencies.

I've also refactored some of the code to make it match the original as most as possible.

Let me know if there is anything else I should do

@rrousselGit
a-hem

You've got lost of packages, I know, whenever you get a chance...

Sorry, let me have another look.

fix: all child dependencies are combined

packages/custom_lint/lib/src/workspace.dart

rrousselGit

Assuming the CI passes, it's looking good!

…into pr/dickermoshe/276

feat: Use linters in regular dependencies

5a21fb6

dickermoshe force-pushed the main branch from a434ce1 to 5a21fb6 Compare September 18, 2024 13:48

test: add tests for linters in regular deps

cb97d93

dickermoshe marked this pull request as draft September 18, 2024 20:02

dickermoshe added 3 commits September 19, 2024 00:26

test: add more tests

dfd3317

feat: add regular deps to workspace

aa9ff49

docs: better doccomments

041f201

dickermoshe marked this pull request as ready for review September 19, 2024 03:55

dickermoshe marked this pull request as draft September 19, 2024 04:01

dickermoshe added 2 commits September 19, 2024 04:36

fix constraints spacing

d7f6f62

small nits

92b88b7

dickermoshe marked this pull request as ready for review September 19, 2024 05:18

fix: don't ignore deps in deps and dev_deps

9209864

dickermoshe force-pushed the main branch from 115c4a7 to 9209864 Compare October 2, 2024 03:28

Merge branch 'main' into main

9455925

rrousselGit reviewed Oct 2, 2024

View reviewed changes

packages/custom_lint/lib/src/workspace.dart Outdated Show resolved Hide resolved

rrousselGit reviewed Oct 2, 2024

View reviewed changes

packages/custom_lint/lib/src/workspace.dart Outdated Show resolved Hide resolved

rrousselGit reviewed Oct 2, 2024

View reviewed changes

fix: workspace installs to dependencies insted of dev_dependencies

06b4928

fix: all child dependencies are combined

dickermoshe and others added 3 commits October 2, 2024 12:29

refactor: make pr simpler to diff

068d22e

test: add tests for conflicting dev and regular dependencies

e5ce9d6

Merge branch 'invertase:main' into main

d10d269

rrousselGit reviewed Oct 10, 2024

View reviewed changes

packages/custom_lint/lib/src/workspace.dart Outdated Show resolved Hide resolved

rrousselGit approved these changes Oct 10, 2024

View reviewed changes

rrousselGit added 5 commits October 10, 2024 17:21

Changelog

718c3ba

Merge branch 'main' into main

d54a33d

Remove unnecessary .map

6c6cd1b

Merge branch 'main' of https://github.com/dickermoshe/dart_custom_lint …

0876034

…into pr/dickermoshe/276

Merge branch 'main' into main

95cc301

rrousselGit merged commit de6546d into invertase:main Oct 10, 2024
4 of 5 checks passed

dickermoshe mentioned this pull request Oct 10, 2024

drift_lint package simolus3/drift#3224

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use linters from regular dependencies #276

feat: Use linters from regular dependencies #276

dickermoshe commented Sep 18, 2024 •

edited

Loading

docs-page bot commented Sep 18, 2024

vercel bot commented Sep 18, 2024

CLAassistant commented Sep 18, 2024 •

edited

Loading

rrousselGit commented Sep 18, 2024

dickermoshe commented Sep 18, 2024

rrousselGit commented Sep 18, 2024

dickermoshe commented Sep 19, 2024 •

edited

Loading

dickermoshe commented Sep 19, 2024

dickermoshe commented Sep 19, 2024

rrousselGit commented Oct 2, 2024

dickermoshe commented Oct 2, 2024

rrousselGit Oct 2, 2024

dickermoshe Oct 2, 2024 •

edited

Loading

rrousselGit Oct 2, 2024

rrousselGit Oct 2, 2024

dickermoshe Oct 2, 2024 •

edited

Loading

dickermoshe Oct 10, 2024

rrousselGit Oct 10, 2024

rrousselGit left a comment

feat: Use linters from regular dependencies #276

feat: Use linters from regular dependencies #276

Conversation

dickermoshe commented Sep 18, 2024 • edited Loading

docs-page bot commented Sep 18, 2024

vercel bot commented Sep 18, 2024

CLAassistant commented Sep 18, 2024 • edited Loading

rrousselGit commented Sep 18, 2024

dickermoshe commented Sep 18, 2024

rrousselGit commented Sep 18, 2024

dickermoshe commented Sep 19, 2024 • edited Loading

dickermoshe commented Sep 19, 2024

dickermoshe commented Sep 19, 2024

rrousselGit commented Oct 2, 2024

dickermoshe commented Oct 2, 2024

rrousselGit Oct 2, 2024

Choose a reason for hiding this comment

dickermoshe Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

rrousselGit Oct 2, 2024

Choose a reason for hiding this comment

rrousselGit Oct 2, 2024

Choose a reason for hiding this comment

dickermoshe Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

dickermoshe Oct 10, 2024

Choose a reason for hiding this comment

rrousselGit Oct 10, 2024

Choose a reason for hiding this comment

rrousselGit left a comment

Choose a reason for hiding this comment

dickermoshe commented Sep 18, 2024 •

edited

Loading

CLAassistant commented Sep 18, 2024 •

edited

Loading

dickermoshe commented Sep 19, 2024 •

edited

Loading

dickermoshe Oct 2, 2024 •

edited

Loading

dickermoshe Oct 2, 2024 •

edited

Loading