feat: add dart_frog_lint package

[rrousselGit]

closes #548

Status

IN DEVELOPMENT

Description

This adds the dart_grog_lint packge, implementing lints proposed in #548

Type of Change

• ✨ New feature (non-breaking change which adds functionality)
• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ❌ Breaking change (fix or feature that would cause existing functionality to change)
• 🧹 Code refactor
• ✅ Build configuration change
• 📝 Documentation
• 🗑️ Chore

[rrousselGit]

Hello! This is the PR about the lint package discussed before.

The lints should be working.
For information, the lints are tested using the // expect_lint comment you see everywhere in the example folder.

I will update the CI to assert that there is no error.
I will also update the Readme with the proper installation steps.

[rrousselGit]

This should be good to review :)

[alestiago]

So far looks good! Thanks for the contribution!

I yet have to deep dive into the logic to see if any edge-cases come up. However, I think we still need to work on the testing strategy for this package.

[alestiago]

Currently we are not testing that the lint rules dart_frog_entrypoint works as expected. In other words, if the DartFrogEntrypoint implementation is altered to have an incorrect behaviour or no behaviour at all, the CI will not complain about it.

We might need another testing strategy for this lint rule? Since we can't really have two main.dart under the same directory?

In here we're using an actual test alongside the example/ testing (note that is very WIP). Despite this, I would still like example/ to include examples of the lint rule failing and being catched by expect_lint.

[rrousselGit]

What do you think about changing filesToAnalyze to:

  @override
  List<String> get filesToAnalyze => ['main.dart', 'dart_frog_entrypoint/**.dart'];

Then we could have example/dart_frog_entrypoint/whatever.dart with expect_lint in it?

[alestiago]

That sounds good to me! However, if someone has a route named dart_frog_entrypoint/index.dart would they get the lint warning? Is there a way to only allow dart_frog_entrypoint/**.dart as a file to analyse only during testing?

[alestiago]

So far LGTM! I left some quick changes to fix some false positives and comment typos. The README looks good to me. The new testing "example" directory structure looks awesome and makes it much easier to debug. Overall the lint logic looks sound to me.

Again, thank you so much for contributing! Looks awesome 💙!

I'm not sure what the team thinks about this testing strategy since the "example" tests are testing the overall functionality (they are great! and we should keep them!) but they do no affect the coverage tooling. Similar to our e2e tests. (cc: @wolfenrain)

[alestiago]

That sounds good to me! However, if someone has a route named dart_frog_entrypoint/index.dart would they get the lint warning? Is there a way to only allow dart_frog_entrypoint/**.dart as a file to analyse only during testing?

[rrousselGit]

Let me try a few things to see if it's possible to modify custom_lint to obtain coverage metrics when using expect_lint.
Although I personally don't use those metrics too often.

[rrousselGit]

From my quick tests, I wasn't able to get test coverage from the expect_lint lines unfortunately.

[tomarra]

Hi @rrousselGit 👋 wanted to check in on this as we are ending the year and are attempting to cleanup PR's. Is this still a priority for you? Should we mark this as draft until you can work on getting things fixed up or close this out for now?

[rrousselGit]

It isn't a priority for me. But is there anything you need from me for it? I don't see a special request