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

[nfc] Fix formatting from early-prototype code #115

Merged
merged 2 commits into from
May 26, 2023
Merged

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented May 26, 2023

Prompted by being about to modify some of the code that's densest with the old patterns, for #72.


fmt [nfc]: Fix basic formatting from early-prototype code

There's a set of formatting patterns we've pretty well settled into at this point. But there's still some swathes of code that don't adhere to them -- mainly from the early prototype period, when we were just figuring out how to write Dart and how we wanted the code to look.

So, here's a quick sweep through our code to apply our current patterns more uniformly.

The major one is indentation: we use 2-space indents throughout, rather than 4-space. This diff therefore looks a lot smaller with git diff -b.

The other one packaged into this same commit is how we close widgets: like ))); at the end of the innermost child, rather than like ),\n),\n);. (For other kinds of function calls we use either form, and I've largely left those in place.)


fmt [nfc]: Adjust newline placement to Flutter style

From the Flutter repo I've picked up a preference to avoid a line break just after = or =>, and in a few similar "cliffhanger" locations. This is paired with a willingness to make some lines longer than 80 columns when that helps readability:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer-a-maximum-line-length-of-80-characters
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#consider-using-for-short-functions-and-methods

We have some such "cliffhanger" newlines in our existing code, much of it in places where the Dart autoformatter put it there before I'd gotten a grasp on what I want to keep from the autoformatter's output and what I don't. So here's a quick sweep through our code fixing those.

Conversely, fix a couple of too-long lines, and mismatched formatting of parallel constructs, that I noticed while doing the sweep.

There's a set of formatting patterns we've pretty well settled into at
this point.  But there's still some swathes of code that don't adhere
to them -- mainly from the early prototype period, when we were just
figuring out how to write Dart and how we wanted the code to look.

So, here's a quick sweep through our code to apply our current
patterns more uniformly.

The major one is indentation: we use 2-space indents throughout,
rather than 4-space.  This diff therefore looks a lot smaller
with `git diff -b`.

The other one packaged into this same commit is how we close
widgets: like `)));` at the end of the innermost child, rather than
like `),\n),\n);`.  (For other kinds of function calls we use either
form, and I've largely left those in place.)
From the Flutter repo I've picked up a preference to avoid a
line break just after `=` or `=>`, and in a few similar "cliffhanger"
locations.  This is paired with a willingness to make some lines
longer than 80 columns when that helps readability:
  https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer-a-maximum-line-length-of-80-characters
  https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#consider-using-for-short-functions-and-methods

We have some such "cliffhanger" newlines in our existing code,
much of it in places where the Dart autoformatter put it there
before I'd gotten a grasp on what I want to keep from the
autoformatter's output and what I don't.  So here's a quick sweep
through our code fixing those.

Conversely, fix a couple of too-long lines, and mismatched formatting
of parallel constructs, that I noticed while doing the sweep.
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, with two tiny nits, I think.

Comment on lines 209 to +210
maxLines: null,
),
),
);
)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Here too, one more newline (and comma) to be removed, I think:

          maxLines: null)));

Comment on lines -200 to 201
maxHeight: 200
maxHeight: 200,
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Instead, wouldn't we normally omit the comma and remove the newline:

          maxHeight: 200),

@gnprice
Copy link
Member Author

gnprice commented May 26, 2023

Thanks for the review!

As we just discussed in the office, those two spots are examples of this:

(For other kinds of function calls we use either form, and I've largely left those in place.)

i.e. of function calls where it's not a widget constructor, or where the last item isn't a child parameter or something of a similar nature, and therefore where the trailing-comma form is fine too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants