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

Enable DartFmt Options with g:dartfmt_options #108

Merged
merged 5 commits into from
Jun 25, 2020

Conversation

theniceboy
Copy link
Contributor

With this PR, users will be able to configure DartFmt options with g:dartfmt_options. They can change the default line limit from 80 to any number they prefer. They can also make dartfmt fix issues such as optional new keywords.

$ dartfmt -h
...
Non-whitespace fixes (off by default):
    --fix                              Apply all style fixes.
    --fix-doc-comments                 Use triple slash for documentation comments.
    --fix-function-typedefs            Use new syntax for function type typedefs.
    --fix-named-default-separator      Use "=" as the separator before named parameter default values.
    --fix-optional-const               Remove "const" keyword inside constant context.
    --fix-optional-new                 Remove "new" keyword.
    --fix-single-cascade-statements    Remove unnecessary single cascades from expression statements.

Other options:
-l, --line-length                      Wrap lines longer than this.
                                       (defaults to "80")
-i, --indent                           Spaces of leading indentation.
                                       (defaults to "0")
...

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

README.md Outdated
@@ -79,6 +79,11 @@ Enable Dart style guide syntax (like 2-space indentation) with

Enable DartFmt execution on buffer save with `let g:dart_format_on_save = 1`

Configure DartFmt Options with `let g:dartfmt_options`, for example, change
line limit to 100 (default 80) and enable auto syntax fixes with
Copy link
Member

Choose a reason for hiding this comment

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

If we merge this we wouldn't want to encourage usage of setting the line limit. Idiomatic usage for dartfmt is to accept the default. We might even want to leave it out of the README entirely since most users shouldn't care about it, and add a note to the doc instead. cc @cbracken - what are you thoughts on how much to put in the README?

I can see the value in supporting this for the --fix arguments, but I'm on the fence about it. I think we typically expect those to be used infrequently, rather than on every run...

Copy link
Contributor Author

@theniceboy theniceboy May 25, 2020

Choose a reason for hiding this comment

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

Personally, I find the 80 line limit annoying, and it looks like there are other people who have the same issue (check out this issue from the dartfmt repo).

I did a search and found that ~35% of our organization's Dart projects (maintained by various independent teams) use a custom dartfmt line length of either 100 or 120 as opposed to the default 80.

Maybe we should explain it more and recommend the default 80 line limit.

For --fix, I find it helpful but it does increase the :wq lag by a little bit. I guess we want to make that clear.

@natebosch

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should explain it more and recommend the default 80 line limit.

I think it's fine for folks to use dartfmt -h to discover the options, we don't need to call them out specifically here. We can mention --fix as the motivating use case in our docs.

I do think this should be reserved for the help docs over the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.

Copy link
Member

@natebosch natebosch left a comment

Choose a reason for hiding this comment

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

If you want to follow the links from the CLA bot and sign the CLA I can help out with getting this documented and merged.

README.md Outdated
@@ -79,6 +79,11 @@ Enable Dart style guide syntax (like 2-space indentation) with

Enable DartFmt execution on buffer save with `let g:dart_format_on_save = 1`

Configure DartFmt Options with `let g:dartfmt_options`, for example, change
line limit to 100 (default 80) and enable auto syntax fixes with
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should explain it more and recommend the default 80 line limit.

I think it's fine for folks to use dartfmt -h to discover the options, we don't need to call them out specifically here. We can mention --fix as the motivating use case in our docs.

I do think this should be reserved for the help docs over the README.

@theniceboy
Copy link
Contributor Author

@natebosch I have signed the CLA with the same email like the one in this commit:
Screen Shot 2020-06-08 at 6 23 08 PM

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 8, 2020
@natebosch
Copy link
Member

Hmm sorry to bikeshed further but now I'm wondering if it would be better to take this as a list instead of a string so we can let vim handle the escaping stuff. It might be breaking if we change this later so I do want to give it some thought up front.

@cbracken - what do you think?

@theniceboy
Copy link
Contributor Author

theniceboy commented Jun 8, 2020

I do think it makes more sense to pass this option as a list, but frankly, I'm not really familiar with vimscript to implement the escape handling stuff... Sorry about that... @natebosch

@natebosch
Copy link
Member

I do think it makes more sense to pass this option as a list, but frankly, I'm not really familiar with vimscript to implement the escape handling stuff

No problem!

If you'd like I can push a change that switches it to a list.

@theniceboy
Copy link
Contributor Author

@natebosch Yes, that would be great! Thanks!

natebosch added 2 commits June 8, 2020 16:21
- Use f-args instead of q-args and use varags for the funciton so all
  arguments are lists.
- Add a missing `shellescape` in case there are characters in the
  filename that need to be escaped.
- Extend the default args with the dartfmt_options args, and the
  manually supplied arguments to the command
@natebosch natebosch requested review from cbracken and sigmundch June 8, 2020 23:26
@natebosch natebosch merged commit b9fd9d2 into dart-lang:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants