Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add dart_frog_lint package #559

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

rrousselGit
Copy link

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
Copy link
Author

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.

@felangel felangel changed the title Feat: Add dart_grog_lint package Feat: Add dart_frog_lint package Mar 2, 2023
@felangel felangel added the feature A new feature or request label Mar 2, 2023
@felangel felangel changed the title Feat: Add dart_frog_lint package feat: add dart_frog_lint package Mar 2, 2023
@rrousselGit rrousselGit marked this pull request as ready for review March 7, 2023 14:36
@rrousselGit
Copy link
Author

This should be good to review :)

Copy link
Contributor

@alestiago alestiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

packages/dart_frog_lint/README.md Show resolved Hide resolved
Comment on lines +1 to +7
import 'dart:io';

import 'package:dart_frog/dart_frog.dart';

Future<HttpServer> run(Handler handler, InternetAddress ip, int port) {
throw UnimplementedError();
}
Copy link
Contributor

@alestiago alestiago Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@alestiago alestiago Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

packages/dart_frog_lint/lib/src/types.dart Outdated Show resolved Hide resolved
alestiago
alestiago previously approved these changes Apr 4, 2023
Copy link
Contributor

@alestiago alestiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

packages/dart_frog_lint/lib/src/types.dart Outdated Show resolved Hide resolved
packages/dart_frog_lint/lib/src/types.dart Outdated Show resolved Hide resolved
packages/dart_frog_lint/example/main.dart Show resolved Hide resolved
Comment on lines +1 to +7
import 'dart:io';

import 'package:dart_frog/dart_frog.dart';

Future<HttpServer> run(Handler handler, InternetAddress ip, int port) {
throw UnimplementedError();
}
Copy link
Contributor

@alestiago alestiago Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

packages/dart_frog_lint/test/src/parse_route_test.dart Outdated Show resolved Hide resolved
packages/dart_frog_lint/lib/src/dart_frog_entrypoint.dart Outdated Show resolved Hide resolved
packages/dart_frog_lint/lib/src/dart_frog_entrypoint.dart Outdated Show resolved Hide resolved
packages/dart_frog_lint/lib/src/dart_frog_entrypoint.dart Outdated Show resolved Hide resolved
packages/dart_frog_lint/lib/src/dart_frog_middleware.dart Outdated Show resolved Hide resolved
packages/dart_frog_lint/lib/src/dart_frog_route.dart Outdated Show resolved Hide resolved
@rrousselGit
Copy link
Author

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
Copy link
Author

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

@renancaraujo renancaraujo self-assigned this May 2, 2023
@tomarra
Copy link
Contributor

tomarra commented Dec 19, 2023

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
Copy link
Author

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

@rrousselGit rrousselGit requested a review from a team as a code owner February 7, 2024 14:23
@tomarra tomarra added the p3 Issues that we currently consider unimportant label Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature or request p3 Issues that we currently consider unimportant
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

feat: custom lints
6 participants