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.

Sign up for GitHub

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

Jump to bottom

Switch to xo + prettier #1080

Merged
merged 4 commits into from
Jan 3, 2020
Merged

Switch to xo + prettier #1080

merged 4 commits into from
Jan 3, 2020

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Dec 16, 2019 •
edited
Loading

By submitting this pull request, I confirm the following: {please fill any appropriate checkboxes, e.g: [X]}

{Please ensure that your pull request is for the 'devel' branch!}

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

With this in place we'll have proper JS linting. It will still use prettier for the formatting part.

Notes:

  1. This requires my other JS PRs merged/sorted out first and also replaces the JS part of Prettify codebase #1079
  2. We should have CI to run this otherwise consistency isn't guaranteed
  3. There are still some errors that I will either disable or try to fix before this PR is ready for review
  4. I will rebase this as soon as the other PRs are sorted. Also, viewing the non-whitespace diff might help, but being that the formatting changes are a lot, it will still be hard to review. That being said, since they are automatically made with prettier, we should be OK

Closes #1090

XhmikosR
XhmikosR commented Dec 16, 2019
View reviewed changes
package.json Outdated Show resolved Hide resolved
package.json
},
"homepage": "https://github.com/pi-hole/AdminLTE#readme",
"scripts": {
"xo": "xo",
Copy link
Contributor Author

@XhmikosR XhmikosR Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

One can autofix some of the issues with npm run xo -- --fix. If you prefer to have a separate script for this let me know how you want it named and I'll add it.

Copy link
Member

Choose a reason for hiding this comment

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

Should this be something we encourage contributors to do, or something that CI just does and pushes back to the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is useful for people fiddling with the source code. CI will just run the normal script

@XhmikosR XhmikosR force-pushed the xo branch 5 times, most recently from 2ec3cb9 to 798efc5 Compare December 16, 2019 19:57
@XhmikosR XhmikosR mentioned this pull request Dec 17, 2019
Merged
9 tasks
@XhmikosR
Copy link
Contributor Author

This will be ready after my open PRs are merged and I rebase this. So what's left is to sort out my comments, namely CI.

@XhmikosR XhmikosR force-pushed the xo branch 3 times, most recently from 84c7a55 to 934e4c6 Compare December 17, 2019 15:01
@XhmikosR XhmikosR mentioned this pull request Dec 17, 2019
Merged
9 tasks
This was referenced Dec 18, 2019
Merged
Merged
@XhmikosR XhmikosR changed the title [WIP] Switch to xo + prettier Switch to xo + prettier Dec 19, 2019
@XhmikosR XhmikosR marked this pull request as ready for review December 19, 2019 18:59
@XhmikosR XhmikosR force-pushed the xo branch 2 times, most recently from e66b201 to 0034d10 Compare December 26, 2019 15:51
@XhmikosR XhmikosR mentioned this pull request Dec 28, 2019
Closed
@PromoFaux
Copy link
Member

This, I'm assuming, adds a development dependency that we did not have before, right? Namely npm

Regards point 2, presumably this is something we can knock together on travis? What scripts would it need to run?

@XhmikosR XhmikosR force-pushed the xo branch 3 times, most recently from 53f83a2 to f805da9 Compare December 28, 2019 16:09
@XhmikosR
Copy link
Contributor Author

We can just use GitHub Actions CI, no need to add Travis CI.

And yeah this adds Node.js as a development dependency, which I guess #1079 requires too? (and the new web repo)

Do not that I have not tested this with any browser, nor the current devel branch with IE

@PromoFaux
Copy link
Member

Actions is all voodoo to me (actually, so is any CI, if I'm being honest!)

Happy to enable stuff on the repo, but will need some pointers...

@XhmikosR
Copy link
Contributor Author

It's enabled, I can add GItHub Actions CI support here. Please also check my comment above about autofix (CC @DL6ER in case you want this or not)

@XhmikosR
Copy link
Contributor Author

The second error might be related to the first, though. We need to fix padStart and see if the second error occurs. IE is weird in general...

@XhmikosR XhmikosR mentioned this pull request Dec 28, 2019
Closed
9 tasks
@PromoFaux
Copy link
Member

Perhaps a polyfill, per MDN?

@XhmikosR
Copy link
Contributor Author

I think we should keep this simple. Yes, we could use a polyfill, but this one case should be easy to work around by rewriting the code.

IMHO we shouldn't start adding polyfills. We just need to make sure we don't introduce any further incompatibilities and this PR will at least catch any ES5+ code, plus it will ensure consistency which is what #1079 does, but this is only for the JS part.

@XhmikosR
Copy link
Contributor Author

Anyway, let's leave padStart for another PR. It might be simpler to add the polyfill, but if we know the number of characters we need to pad, we could probably use slice and construct the new string

@PromoFaux PromoFaux force-pushed the xo branch 2 times, most recently from 5aa09b5 to d1c612b Compare December 28, 2019 17:13
@XhmikosR XhmikosR force-pushed the xo branch 2 times, most recently from 2c5c381 to 57d6498 Compare December 28, 2019 17:21
This was referenced Dec 29, 2019
Merged
Merged
Merged
Merged
Merged
@XhmikosR
Copy link
Contributor Author

I split most of the non-stylistic changes to separate PRs. Let's keep this on hold until everything else is merged and I'll rebase and finally we can move on with this.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 2, 2020

Can we get #1110, #1111, #1112, #1113, #1114 merged? Then we can merge this patch after I rebase it.

@PromoFaux
Copy link
Member

All merged, go ahead.

@XhmikosR
Add xo.
3321fa8 
Use prettier for formatting.

Signed-off-by: XhmikosR <[email protected]>
@XhmikosR
Format JS code
704c352 
Signed-off-by: XhmikosR <[email protected]>
@XhmikosR
Fix the remaining lint issues
b74f0f3 
Signed-off-by: XhmikosR <[email protected]>
@XhmikosR
Add GitHub Actions CI support.
d29ac3a 
Signed-off-by: XhmikosR <[email protected]>
@PromoFaux
Copy link
Member

@XhmikosR I'm happy to merge this if you are happy that it is ready?

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jan 3, 2020

This is ready from my side. Just keep in mind that any other JS PRs will need to be rebased and have them formatted.

@PromoFaux PromoFaux merged commit 0a16b94 into devel Jan 3, 2020
@PromoFaux PromoFaux deleted the xo branch January 3, 2020 12:40
@DL6ER DL6ER mentioned this pull request Feb 21, 2020
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PromoFaux PromoFaux PromoFaux approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@XhmikosR @PromoFaux