Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

Add prettier. Closes #466 #481

Merged
merged 4 commits into from
Jan 31, 2018
Merged

Add prettier. Closes #466 #481

merged 4 commits into from
Jan 31, 2018

Conversation

juliangruber
Copy link
Collaborator

based upon #480, this will help us even more with consistency across all contributors.

Some of prettier's rules can be tweaked as well, but for now I value consistency a lot over the "most optimal way" of writing a line of code.

@juliangruber juliangruber changed the title Add prettier Add prettier. Closes #466 Jan 29, 2018
Copy link
Collaborator

@martinheidegger martinheidegger left a comment

Choose a reason for hiding this comment

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

I am not sure prettier-standard is overkill.

.then(() => endTest(app))
})

// Create a new app instance
function createApp () {
return new spectron.Application({
path: path.join(__dirname, '../node_modules/.bin/electron'),
args: [path.join(__dirname, '../index.js'), '--data', TEST_DATA, '--db', TEST_DATA_DB],
args: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it just me or has this become significantly less readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it's not perfect every single time, but it's consistent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

before, this line was 91 chars long, limiting lines to 80 chars is a common practice

@@ -12,83 +12,88 @@ tap('init', function (t) {
var app = createApp()
return waitForLoad(app)
.then(() => app.browserWindow.isVisible())
.then((val) => t.ok(val, 'isVisible'))
.then(val => t.ok(val, 'isVisible'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noticed that the #453 needs to be merge into this.

}).then(function () {
return app.client.waitUntilWindowLoaded()
})
return app
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also seems way more stringent in the former version.

@juliangruber juliangruber mentioned this pull request Jan 31, 2018
@juliangruber
Copy link
Collaborator Author

I'm going to make a move on this now because I don't want to have to deal with too many merge conflicts and value having a formatter a lot. The success of prettier and languages like go that have this built on speaks about the advantages.

Not having to argue about the best way of writing something ever again, in a collaborative environment, is usually worth more than that one perfect way of writing a code block.

@juliangruber juliangruber merged commit d275d2b into react Jan 31, 2018
@juliangruber juliangruber deleted the add/prettier branch January 31, 2018 11:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants