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

Proper linting #1058

Merged
merged 27 commits into from
May 2, 2018
Merged

Proper linting #1058

merged 27 commits into from
May 2, 2018

Conversation

lipis
Copy link
Contributor

@lipis lipis commented Oct 17, 2017

No description provided.

lipis added 5 commits October 18, 2017 17:00
* 'lint' of github.com:wireapp/wire-desktop:
  chore: Update version to 3.1 (#1068)
  chore: Update electron to version 1.7.9 (#1067)
@lipis lipis mentioned this pull request Oct 20, 2017
@lipis
Copy link
Contributor Author

lipis commented Oct 20, 2017

Related to the error: eslint/eslint#8767

@lipis
Copy link
Contributor Author

lipis commented Dec 6, 2017

Can we deal with this one somehow?

/Users/lipis/work/wire/wire-desktop/electron/main.js
  127:5  error  Parsing error: 'return' outside of function

✖ 1 problem (1 error, 0 warnings)

https://github.com/wireapp/wire-desktop/blob/lint/electron/main.js#L127-L129

@gregor
Copy link
Contributor

gregor commented Jan 11, 2018

@lipis Is this still an issue with prettier 1.10.2?

@lipis
Copy link
Contributor Author

lipis commented Jan 11, 2018

it's not the prettier.. but the Eslint.. for the retrun outside of a function

@gregor
Copy link
Contributor

gregor commented Jan 12, 2018

Thanks for the clarification. Can't we simply add a single line ignore on this one to move forward with code formatting?

@lipis
Copy link
Contributor Author

lipis commented Jan 12, 2018

No it doesn't work as it's not linting issues.. is parsing error.. I've read the whole internet the other day (twice) to skip this line.. but couldn't figure out a way.. and since the behaviour is affecting the Windows update I couldn't test it properly in case of changing the code.

electron/main.js Outdated
@@ -439,7 +439,7 @@ fs.readdir(LOG_DIR, (error, contents) => {
.filter(file => {
try {
return fs.statSync(file).isFile();
} catch (error) {
} catch (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the more readable variable, 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.

To avoid variable shadowing.. maybe error_?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes. Then I think better would be statError.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you can also rename the first error to readError :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

much better..

@@ -1,3 +1,4 @@
/* eslint-disable sort-keys */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why disable sorting keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they were too many and I wanted to do it once this PR is merged to avoid future conflicts.. Maybe I'll add a TODO to do it in a separate PR after we merge this .

@lipis
Copy link
Contributor Author

lipis commented Apr 25, 2018

What if we replace the line https://github.com/wireapp/wire-desktop/blob/master/electron/main.js#L128 with app.exit()?

electron/main.js Outdated
@@ -416,7 +416,7 @@ class ElectronWrapperInit {
}
};

app.on('web-contents-created', (event, contents) => {
app.on('web-contents-created', (createdEvent, contents) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ffflorian if you have a better name.. let me know! because of the shadowing the line 422

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it

electron/main.js Outdated
@@ -425,7 +416,7 @@ class ElectronWrapperInit {
}
};

app.on('web-contents-created', (event, contents) => {
app.on('web-contents-created', (createdEvent, contents) => {
switch (contents.getType()) {
case 'window':
contents.on('will-attach-webview', (event, webPreferences, params) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

webviewEvent

@lipis lipis merged commit a81e508 into master May 2, 2018
@lipis lipis deleted the lint branch May 2, 2018 11:49
@gregor
Copy link
Contributor

gregor commented May 2, 2018

Nice

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.

3 participants