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

Add prettier code formatting #377

Merged
merged 17 commits into from
Mar 5, 2019
Merged

Conversation

jason-fox
Copy link
Contributor

Adds prettier and husky. All JavaScript files are autoformated on commit using the Git Hooks hook.

running npm run prettier will format all files with standardized whitespace.

parameters: ['host', 'port', 'apiKey', 'deviceId'],
description: '\tConfigure the client to emulate the selected device, connecting to the given host.',
description:
'\tConfigure the client to emulate the selected device, connecting to the given host.',
Copy link
Member

Choose a reason for hiding this comment

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

I have a kind of deja vu looking to this... :)

We were discussing about line length in telefonicaid/perseo-fe#314 (comment) and we ended with "printWidth": 120 (https://github.com/telefonicaid/perseo-fe/blob/master/.prettierrc.json#L6). I think this setting should be common to this an other similar opened PRs.

Copy link
Member

Choose a reason for hiding this comment

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

(I didn't realize in the IOTA Sigfox onboarding PR... the 100 at https://github.com/telefonicaid/sigfox-iotagent/blob/master/.prettierrc.json#L6 probably should be 120 but it's fine given that the PR there is already merged).

Copy link
Contributor Author

@jason-fox jason-fox Feb 23, 2019

Choose a reason for hiding this comment

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

Fixed f90c273 - I think I must have missed that one in the config, the others should be 120.

@@ -0,0 +1,4820 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

IOTA repositories intentionally doesn't include package-lock.json. We can open a discussion thread about the convenience of having pacakges-lock.json included in the repository or not :) but by the moment I'll suggest to leave it out this PR, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Package lock removed. Fixed 6f5a131

rejectUnauthorized: true,
username: null
}).should.equal(true);
mqtt.connect
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I like this... too "sparse", don't you think?

On the contrary

mqtt.connect.calledOnceWithExactly({

looks nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 7657825

'/' + apiKey + '/' + deviceId + '/' + constants.CONFIGURATION_SUFIX + '/' +
constants.CONFIGURATION_VALUES_SUFIX,
JSON.stringify(configurations), options, callback);
'/' +
Copy link
Member

Choose a reason for hiding this comment

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

Topic/URL composition is better in just one line. Could this change be avoided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 7657825 - Note the line is over 120 chars unless split once anyway.

this.code = 400;
},
UnsupportedType: function(expectedType) {
this.name = 'UNSUPPORTED_TYPE';
this.message = 'The request content didn\'t have the expected type [' + expectedType + ' ]';
Copy link
Member

Choose a reason for hiding this comment

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

This should be morer compact. Maybe using [%s] it could be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 7657825

parameters: ['attributes'],
description: '\tSend a collection of attributes to the MQTT broker, using JSON format. The "attributes"\n' +
Copy link
Member

Choose a reason for hiding this comment

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

This line is 117 chars long. I wonder why prettier has changed it if printWidth is 120...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably prettier picks up on the \n and breaks at that point

Copy link
Member

Choose a reason for hiding this comment

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

A bit weird... but ok.

NTC

Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Passing the ball to @dcalvoalonso for additional LGTM before merging

@dcalvoalonso
Copy link
Contributor

LGTM! Thanks @jason-fox !

Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan fgalan merged commit 59649a2 into telefonicaid:master Mar 5, 2019
@jason-fox jason-fox deleted the feature/prettier branch March 18, 2019 18:54
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.

4 participants