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

UIP-1639 Update formatting guidelines WRT dartfmt and trailing commas #37

Merged
merged 3 commits into from
Jan 6, 2017

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Jan 3, 2017

Ultimate problem:

Formatting guidelines were outdated when it came to trailing commas. (#25)

How it was fixed:

Update them.

Testing suggestions:

  • Verify that the new guidelines are correct and makes sense.

Potential areas of regression:

  • README formatting

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

@aviary-wf
Copy link

Raven

Number of Findings: 0

@codecov-io
Copy link

codecov-io commented Jan 3, 2017

Current coverage is 97.44% (diff: 100%)

Merging #37 into master will not change coverage

@@             master        #37   diff @@
==========================================
  Files            27         27          
  Lines          1286       1286          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1253       1253          
  Misses           33         33          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update b533cd2...2fb8625

>
> _We have a long-term goal of writing our own formatter that extends from `dartfmt` to make it possible for our
> fluent-style to remain readable even after the formatter executes._
#### dart_style
Copy link
Contributor

Choose a reason for hiding this comment

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

#nit shouldn't this be an h3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk, I made it an h4 so it'd look less important 😄 . Didn't want to take away from the meat of this section: the formatting guidelines.

I can change it to be more consistent, though

Copy link
Contributor

Choose a reason for hiding this comment

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

@greglittlefield-wf I'd say - to make it look less important - just use bold text instead of a heading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 2fb8625

@aaronlademann-wf
Copy link
Contributor

+1

@jacehensley-wf
Copy link
Contributor

+1

@leviwith-wf
Copy link
Contributor

QA +10

  • readme formatting looks fine
  • New guidelines read well

merging.

@leviwith-wf leviwith-wf merged commit 7441ce6 into Workiva:master Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants