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

Refactor function prototypes to classes, use arrow function instead of bind #19

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ristomatti
Copy link
Collaborator

For discussion - this is my old PR to MariusRumpf/node-lifx I mentioned here: #15 (comment) rebased to master.

I haven't tested yet if it even runs but I'd like to hear what you think about the concept @Sawtaytoes? In any case this shouldn't be merged before any other open PR.

@ristomatti ristomatti added the wip Work in progress label Oct 5, 2019
@ristomatti
Copy link
Collaborator Author

To be able to rebase this with the least amount of effort, I used IntelliJ IDEA as it's the IDE I use for work and has the best merge conflict diff view I've seen. The experience was quire enlightening by other means also. Pretty much all the files generated a lot of warnings based on IDEA's code analysis. Some of them looked like potential bugs. It also spotted some unused functions and variables that could be cleaned up.

Earlier on I've wanted to make a psychological distinction between coding for work and on my free time, so I've done personal projects with VSCode. I'm thinking of making an exception with this one.

If only we could switch to TypeScript, working with and understanding this code would become so much easier. ;)

@Sawtaytoes
Copy link
Collaborator

Since this is approved, do you want to merge it?

@ristomatti
Copy link
Collaborator Author

@Sawtaytoes Hmm I'm curious why I marked this as WIP. Maybe it was as I thought the Tile PR should be merged first? Let's wait a while if there's some activity there as it'd create a mess for him to first merge this. It's easier for me to update this PR to also handle his changes or what do you think? If there's no activity, you're welcome to merge this yourself. 🙂

@Sawtaytoes
Copy link
Collaborator

I'm fine waiting if we want to avoid conflicts with another PR.

@ristomatti ristomatti added the dependencies Pull requests that update a dependency file label Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants