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

Lint & refactor public/js/main.js #298

Closed
wants to merge 1 commit into from

Conversation

@MattIPv4
Copy link
Member

commented Sep 10, 2019

This resolves #297 by updating the test script to ensure ESLint checks public/js/main.js
This also refactors the whole file to give us a cleaner base to work from going forward.

@MattIPv4 MattIPv4 self-assigned this Sep 10, 2019

@MattIPv4 MattIPv4 added the 🧪 tests label Sep 10, 2019

@MattIPv4 MattIPv4 force-pushed the MattIPv4-eslint-public branch from 0f2b3d2 to 0236e19 Sep 10, 2019

@MattIPv4 MattIPv4 changed the title 🚧 ESLint & refactor public/js/main.js Lint & refactor public/js/main.js Sep 10, 2019

@MattIPv4

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Sorry for the awful diff all. I'm super confident everything is working as intended on the front-end still after lots of testing, but I am happy to be proven wrong and then fix it.

@MattIPv4 MattIPv4 requested review from drewfreyling and PeterDaveHello Sep 10, 2019

@PeterDaveHello
Copy link
Contributor

left a comment

Awesome works 👍 Make the things more readable and maintainable, very nice.

As usual, I will suggest to split eslint config into another PR, and separate different types of changes, like comment, indent, wrap, etc, to make it more reviewable and traceable. Thanks a lot.

@MattIPv4

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

@PeterDaveHello I'm happy to move the eslint change to its own commit for traceability but would like to keep in it this PR as it was a style rule that was reflected in this code originally and is still in place in the refactored code, I don't see a reason to move it to a separate PR.

As for splitting the changes by type, I could go in and attempt to do this now, but when I refactored this, I pretty much rewrote the file from scratch, so there isn't really a good way to split it up. Splitting by "section" would be almost impossible as so much of the old code was interlinked.

I realise it's an absolute nightmare to review it as one commit, but I think, in this case, that's going to be the easiest thing to do. Sorry! :(

@PeterDaveHello

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

The linter config change is not reflected by the PR topic, and I believe that config should and would also be appied to the other js files, not only public/js/main.js, that's why it should be separated.

The other changes should be separated from the beginning, refactoring is really nice, but the process is as important as the refactoring, can't be done by a such huge commit which is not tracable and reviewable.

To be honest, effort of reviewing this commit will be times of splitting it, and also the future tracing effort, we can reduce the effort of future, so please let's do it the right way, with the already done right things here ;)

@PeterDaveHello

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

btw, I also see some naming change / renaming changes, that's something should be separated, thank you!

@MattIPv4

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

@PeterDaveHello I don't understand how I would split this into like comment, indent, wrap, etc - I didn't do this when I refactored the file, I rewrote it from scratch?

@PeterDaveHello

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@MattIPv4 From my past experiences, yes, for this huge change, rewrite it from scratch would be easier, as they are too big to be just committed separately, the different thing is - this time, you have refactorred result as reference, only need to focus on how to split them, and the result would be really helpful.

@MattIPv4

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

Right, but as I've rewritten it from scratch, many things don't track over and I can't just "split it up". So many things were interlinked in the old version and some still are in the new version, there is no logical way to split this up.

if you see a way to split this up as you have requested, I invite you to do so. I do not see a way to do this and can only split this per "section", which is what I had done prior to squashing as you normally would request everything is squashed.

@MattIPv4 MattIPv4 closed this Sep 11, 2019

@MattIPv4 MattIPv4 deleted the MattIPv4-eslint-public branch Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.