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

Automatic format! #545

Merged
merged 3 commits into from
Apr 11, 2018
Merged

Automatic format! #545

merged 3 commits into from
Apr 11, 2018

Conversation

e111077
Copy link
Member

@e111077 e111077 commented Apr 10, 2018

Runs the experimental autoformatter webmat which runs clang-format on js files and makes it so that it can run on HTML files. Please look through this PR with ?w=1 in the diff to make sure that no logic was changed.

What has changed?
You can run the formatter on the whole project by running npm run format and ex/include files from the formatter, and enter your custom clang-format config in a formatconfig.json see webmat readme for more

Polymer.AppScrollEffectsBehavior,
Polymer.IronResizableBehavior
],
behaviors: [Polymer.AppScrollEffectsBehavior, Polymer.IronResizableBehavior],
Copy link
Contributor

Choose a reason for hiding this comment

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

At what point does the formatter decide to have each item on its own line (behaviors, listeners, and properties)? This seems like it should be.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's using google styleguide, so I think it's after it hits 80 cols


interface HTMLElementTagNameMap {
"app-drawer": AppDrawerElement;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The typings for app-drawer are now missing

Copy link
Member Author

Choose a reason for hiding this comment

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

that. is. not. supposed. to. happen. 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

ah this is because the missing semicolons caused a typescript syntax error

@@ -283,8 +255,9 @@
this._boundEscKeydownHandler = this._escKeydownHandler.bind(this);
this.addEventListener('keydown', this._tabKeydownHandler.bind(this))

// Only listen for horizontal track so you can vertically scroll inside the drawer.
this.listen(this, 'track', '_track');
// Only listen for horizontal track so you can vertically scroll
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semicolon above is causing this indentation.

var t = this._FLING_INITIAL_SLOPE * dx / velocity
this._styleTransitionDuration(t);
var t = this._FLING_INITIAL_SLOPE * dx /
velocity this._styleTransitionDuration(t);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semicolon

* whereas the rest of the elements will collapse below `sticky` element.
* That is, the `sticky` element remains visible when the header is fully
*condensed whereas the rest of the elements will collapse below `sticky`
*element.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no space between * and text. Not a blocker, but curious why linter doesn't handle this.

Copy link
Member Author

@e111077 e111077 Apr 10, 2018

Choose a reason for hiding this comment

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

odd.. it should clang-format does not

*
* ```html
* <link rel="import" href="/bower_components/app-layout/app-scroll-effects/app-scroll-effects.html">
* <link rel="import"
* href="/bower_components/app-layout/app-scroll-effects/app-scroll-effects.html">
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be an issue with the README generator (i.e. will it remove the * in the HTML block), but maybe it's ok?

Copy link
Member Author

@e111077 e111077 Apr 10, 2018

Choose a reason for hiding this comment

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

I can tell it to not reflow comments do you think that's best? Otherwise clang-format does not support adding the space before

Copy link
Member Author

Choose a reason for hiding this comment

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

Sheesh, nevermind, that's a bad bad flag
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better if it enforces the line limits on all comments - code snippets in comments are a small use case, and I'm not even sure if the README generator is used on this repo anyways. Suggest you ignore this issue for now, and we'll deal with it if it's an issue later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make an issue in webmat, but I think this is an issue in the clang-format blackbox

@e111077
Copy link
Member Author

e111077 commented Apr 10, 2018

fixes applied

Copy link
Contributor

@keanulee keanulee left a comment

Choose a reason for hiding this comment

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

Please update typing to get tests running on travis:

ERROR: Typings are stale. Please run "npm run update-types".

@e111077
Copy link
Member Author

e111077 commented Apr 10, 2018

I was testing you

Copy link
Contributor

@dfreedm dfreedm left a comment

Choose a reason for hiding this comment

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

LGTM for travis config

Copy link
Contributor

@keanulee keanulee left a comment

Choose a reason for hiding this comment

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

Retrying flaky test on IE 11 on Travis. LGTM otherwise.

@keanulee
Copy link
Contributor

LGTM

@e111077 e111077 merged commit e62995a into master Apr 11, 2018
@e111077 e111077 deleted the auto-cleanup branch April 11, 2018 19:35
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.

None yet

5 participants