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

Running custom_lint on a single file #80

Open
literalEval opened this issue Feb 15, 2023 · 23 comments
Open

Running custom_lint on a single file #80

literalEval opened this issue Feb 15, 2023 · 23 comments
Assignees
Labels
enhancement New feature or request

Comments

@literalEval
Copy link

Thanks for this awesome plugin. I recently integrated in my org's project and the custom lint is working like charm. Uses quite some memory but that's a different issue.
I wanted to ask is there any way to lint a single file ? Like from command line ?

Describe what scenario you think is uncovered by the existing examples/articles

Currently, following the documentation, I am able to run flutter pub run custom_lint in the project root and it scans all the files of the project. I couldn't find any documentation which states how to lint a single file from command line.

Additional context
We are working on a team project in an org and want to add Documentation compliance for new files. As all the existing files can't be documented at once, we want that any new PR should include complete documentation for the changed files.
If it was possible to run custom_lint on a single file, then this task could easily be integrated to GitHub workflow of our project.

@literalEval literalEval added the documentation Improvements or additions to documentation label Feb 15, 2023
@rrousselGit
Copy link
Collaborator

There's no such thing yet. It could be added though

@rrousselGit rrousselGit added enhancement New feature or request and removed documentation Improvements or additions to documentation labels Feb 15, 2023
@literalEval
Copy link
Author

Thanks for clarifying @rrousselGit . If I submit a PR that adds this features, will it be merged ?

@rrousselGit
Copy link
Collaborator

If it's tested, sure

@literalEval
Copy link
Author

Cool

@literalEval
Copy link
Author

literalEval commented Feb 16, 2023

@rrousselGit I looked at the codebase a bit and realized that It is tightly made to work only on whole packages. I also tried running Dart's official linter and the main difference is that the latter runs the linter on a single file basis unlike custom_lint.
I couldn't find anything in the code which shows place to change the 'package centric' architecture of custom_lint. Am I correct ? If no, can you please guide me about where to look in the codebase to implement this feature ?

For now, I propose a solution that doesn't break this plugin's architecture, while allowing the user to take only as much information as they need.

  1. Extending the options list to accept files.

image

  1. When getting response from the server, only store information about the files which are in the files list (files can be stored in a Hash Map to optimize this retrieval time) and neglect all the other information. Something like

image

What do you think ? Please make suggestions so that I can improve on my idea. Thanks.

@rrousselGit
Copy link
Collaborator

I don't think this belongs in the options object. I don't it makes sense either to analyze everything but only show the output of a few files

To implement this there's probably two sides:

  • On custom_lint's side, you can change the logic related to searching "context roots" in the /bin file
  • you can also have custom_lint pass the file args to custom_lint_builder (cf other params like watchMode & includeBuiltIntLints)
  • on custom_lint_builder's side, you could change the analyzeFile function to do nothing if a file is not within the option arg

@literalEval
Copy link
Author

literalEval commented Feb 16, 2023

Hmm. Correct.

I did find analyzeFile in the custom_lint_builder.dart and I understand how to not analyze a file by simply returning from there. But can you please explain in some detail on how to send this fileList data from custom_lint to custom_lint_builder ? I can implement everything from there on. Thanks for your support and time.

@rrousselGit
Copy link
Collaborator

Search for runSocket and look at how watchMode is passed around from the CLI to custom_lint_builder

@literalEval
Copy link
Author

literalEval commented Feb 17, 2023

Thanks. I figured it out and implemented this feature. The changes are -

  1. Users can now enter a list of comma separated file paths which are files from the package they are in.
  2. The client will check whether the user has used this files flag or not and send the files data to the server accordingly.
  3. If fileList is null, then all files will be parsed, otherwise only the files given through --files will be parsed.

If this looked good to you @rrousselGit , I will open the respective PR.

Below are the attached images of working of this feature -

image

Without files flag

image

With files flag

image

@rrousselGit
Copy link
Collaborator

Rather than -f it should probably be custom_lint vs custom_lint file/path another/path

@rrousselGit
Copy link
Collaborator

Make sure folders are supported too :)

@literalEval
Copy link
Author

literalEval commented Feb 17, 2023

@rrousselGit

  1. About the flag, I believe that can be done using taking the rest arguments. But then it will interfere with the
    2.argument being a directory or a file. If we strictly separate with flags, then a clear distinction will be made.
  2. Do you mean that given a specific folder of the package, like say lib/constants, then custom_lint scan everything inside lib/constants ? If so, how about I implement it like - Pass dirList same as fileList and then append the path of files it contains to fileList in the server side.

Also, how about I open a PR with the work done till now, and if you find it workable, I'll add the directory function too. I was just thinking if I change a lot of stuff and you find something problematic, then I'll have to change everything again :)

@rrousselGit
Copy link
Collaborator

About the flag, I believe that can be done using taking the rest arguments. But then it will interfere with the
2.argument being a directory or a file. If we strictly separate with flags, then a clear distinction will be made.

I don't agree.
It's up to your implementation to check whether the path points to a file or a folder. That's how most commands works.

I don't think having a -f/-d makes sense here.

@rrousselGit
Copy link
Collaborator

Do you mean that given a specific folder of the package, like say lib/constants, then custom_lint scan everything inside lib/constants

Yes

But it shouldn't need a specific flag for this.

@literalEval
Copy link
Author

literalEval commented Feb 17, 2023

Okay @rrousselGit . I'll modify the PR real quick :)

@rrousselGit
Copy link
Collaborator

Use dart test as example

You can do dart test, dart test test/my_test.dart or dart test test/folder and all of these work.

You can mix and match them too: dart test test/my_test.dart test/folder

@literalEval
Copy link
Author

literalEval commented Feb 17, 2023

@rrousselGit please look at #82 now.

@literalEval
Copy link
Author

@rrousselGit can we discuss its implementation once again please?

@rrousselGit
Copy link
Collaborator

Oh, sorry for the delay. Sure!

@rrousselGit
Copy link
Collaborator

In the meantime as you might've seen, I made a bunch of refactoring to better handle project parsing.

There's now a CustomLintWorkspace class for decoding a directory. And you can call CustomLintWorkspace.fromPaths to decode a workspace from a list of file/directory paths.

This should take care of half the job (anything inside custom_lint). The other half of the job is in custom_lint_builder to not analyze files that aren't in this list of paths. I believe you've done that before already

@literalEval
Copy link
Author

@rrousselGit my earlier implementation was to just return out of the file analysis function, if the file path does not exist in the list of paths we are expecting. Does this solution still look good to you ?

@rrousselGit
Copy link
Collaborator

Yes that's fine

@literalEval
Copy link
Author

That's great :)

ghost pushed a commit to solid-software/dart_custom_lint that referenced this issue Dec 5, 2023
* Replace DCM with custom lints

* Bring back comment on prefer_match_file_name

---------

Co-authored-by: Yurii Prykhodko <yurii.prykhodko@solid.software>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants