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

consider adding support for fixing (single) file(s) to dart fix #43892

Closed
pq opened this issue Oct 22, 2020 · 9 comments
Closed

consider adding support for fixing (single) file(s) to dart fix #43892

pq opened this issue Oct 22, 2020 · 9 comments
Assignees
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. dart-cli-fix P3 A lower priority bug or feature request

Comments

@pq
Copy link
Member

pq commented Oct 22, 2020

Related to: #43886.

Currently:

[~/src/repos/dart/sdk/pkg/_fe_analyzer_shared] (master) $ dart fix lib/src/parser/identifier_context_impl.dart 

*** The `fix` command is provisional and subject to change or removal in future releases. ***

Directory doesn't exist: lib/src/parser/identifier_context_impl.dart

Usage: dart fix [arguments]
-h, --help    Print this usage information.

Run "dart help" to see global options.

I would expect this to work.

@pq pq added area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. dart-cli-fix labels Oct 22, 2020
@pq
Copy link
Member Author

pq commented Oct 22, 2020

(Failing support, the error message should be improved.)

@devoncarew devoncarew added this to Todo in Dart fix / Flutter fix MVP via automation Nov 9, 2020
@devoncarew devoncarew added the P2 A bug or feature request we're likely to work on label Nov 9, 2020
@devoncarew devoncarew added P3 A lower priority bug or feature request and removed P2 A bug or feature request we're likely to work on labels Nov 18, 2020
@pq pq removed this from Todo in Dart fix / Flutter fix MVP Nov 20, 2020
@bkonyi
Copy link
Contributor

bkonyi commented Mar 18, 2021

@pq do we plan on doing this any time soon? This issue should probably have an owner so it doesn't get lost.

@pq
Copy link
Member Author

pq commented Mar 18, 2021

It's not planned yet. I'll self-assign to keep it alive.

@pq pq self-assigned this Mar 18, 2021
@elektronik2k5
Copy link

@pq any progress? I'm in the process of setting up git hooks for incremental codebase health improvement and am very surprised that dart fix only operates at the directory resolution.

@pq
Copy link
Member Author

pq commented Sep 9, 2021

Thanks for the ping @elektronik2k5 , I've been working mostly elsewhere and haven't dug into this one unfortunately.

/fyi @devoncarew @bwilkerson: maybe this is something to consider when we slate some time for CLI feature work...

@jwren : curious if this has any application internally (or if directories are the expected scope)?

@pq pq removed their assignment Sep 9, 2021
@jwren
Copy link
Member

jwren commented Sep 9, 2021

dart fix operates on directory structure because the directory structure is required to resolve Dart (i.e. up to the nearest .packages file). Given that the work has to be done to resolve the whole directory anyways, I suspect, there is no real time difference for running the algorithm over a single file versus the directory.

With regard git hooks, perhaps the generic rule would be something along the lines of: for each pubspec file, validate that there are no dart fix changes with that CWD.

No one has requested this internally.

@bwilkerson
Copy link
Member

If it becomes interesting to support fixing single files I expect that it won't be difficult. There might be some changes on the CLI side to accept a single file, and there might be some changes on the server side to accept a file, but by the time we go to build an AnalysisContextCollection everything should work fine with one or more files and directories. It seems likely that all we need to do is remove some code that's verifying that we're passing along a directory.

@elektronik2k5
Copy link

elektronik2k5 commented Sep 10, 2021

Thanks a lot for looking into it, @pq, @jwren and @bwilkerson!
Given this limitation, a very crude but quick way to implement file based linting can be:

  1. Collect directories from input files: lib/utils/foo.dart => lib/utils
  2. Filter directories to get only uniques: [lib/utils, lib/utils, lib] => [lib/utils, lib]
  3. Remove subdirectories from the list: [lib/utils, lib] => [lib]
  4. Run current implementation of dart fix per unique directory
  5. Filter output to only contain input files from step 1
  6. ???
  7. PROFIT!!!

With some quick tinkering I got steps 1, 2 and 4 this shell one liner:

dirname {staged_files} | uniq | xargs sh -c 'for directory; do dart fix --apply "$directory"; done' sh

{staged_files} is a placeholder for the result of a shell glob or a list of files.

Any chance of implementing something like this properly, as part of dart fix itself in the near future? Or should I try to add the missing steps myself as a workaround?

@leafpetersen
Copy link
Member

I just ran into this as well. The error message is pretty confusing, especially if you don't use --apply when you first run it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. dart-cli-fix P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

7 participants