refactoring

[lesion]

I was doing an upgrade of the protocol version as specified in #3, but decided to doing the work @Raindeer44 has done in the main armadietto.js file to the other part of the project.
I'm working in feat/refactoring branch.

[raucao]

Cool.

Just as an aside, we have documented branch naming for org repos here: https://remotestoragejs.readthedocs.io/en/latest/contributing/github-flow.html#branch-names -- Makes everyone's lives easier, when things are not completely random. Also contains other tips, e.g. for commit messages.

[lesion]

I've renamed the branch as suggested (feature/11-refactoring). Sorry for constantly breaking rules :)
BTW the new documentation is awesome, you all did a very good job.

[lesion]

DONE:
[x] use es6 class instead of base inherit method
[x] use arrow function when possible
[x] remove all self = this reference (due to arrow function this context)
[x] maintain test still running 😄
[x] linting everythings

TODO:
What do I want to do is to promisify all the code (especially fs methods) and to get rid of all the callback hell. @Raindeer44 what do you think about this? What is your plan with async?
For instance, I would love to also manage errors with promise as currently eslint is complaining about unhandled errors.
If you're ok with this I would merge to master

[thornjad]

I haven't worked with promisify before, but that sounds like a fantastic idea. You mean the package promisify right? For async, I've been basically doing the same thing, though I haven't had a ton of time to devote to it in the last couple days.

I'm strongly in support of this refactoring in general. It makes the code look better, makes it easier to maintain, and can make optimization easier.

[thornjad]

To clarify what I've been doing, I'm replacing async with use of the native async functions, but also using the native Promise API.

Note: I edited to clarify my clarification

[lesion]

I love that and that's the way I would go too.
I did not mean specifically the promisify library, but just a way to avoid using callback and clean up it a bit (in reality, I was meaning the util.promisify stuff)...

Unfortunately refactoring from callback to Promise in node is not that linear (native fs promise support requires node v9.8.0, landed in master few month ago) and await fs.readFile (as used here) does not actually work :(

options I see are:
1- using external libraries that solves the problem (fs-extra)
2- use util.promisify (this could be used also in other modules, e.g. redis)

I would go with the first (search lib supporting promises) when possible and with the second in case.

BTW. I'm using standardjs as coding convention while refactoring, are you ok with this? I used this 'cause it's my everyday linter and I like it but if you wanna switch to airbnb or semistandard both are ok to me

[thornjad]

Oh oh oh! I also have not run into util.promisify but I like that too! I'm a big fan of the idea in general; I've never liked callbacks as much as promises.

For the options you listed, I think I like them in the opposite order, trying to use util.promisify before other libraries, just because in general I'd rather use built-in code than dependencies. But of course that isn't always an available option (like fs-extra seems like a good option since I think we should support node v8.x.x).

---

For coding style, I like standardjs a lot except:

• I want semicolons everywhere. This is because A) I have run into many cases in which they are necessary and B) I'd rather define where the semicolon goes instead of have the compiler guess for me. Plus I think it looks better (at least coming from a C/Java background)

• I don't like spaces after function names. I admit there's no utility here, I just think function foo(arg) looks better than function foo (arg).

Because of the semicolons, I'd rather do semistandard (I can get over the spaces). However, I think it's more important that we all at least mostly agree, so armadietto doesn't get all messy.

[raucao]

Just a note regarding spaces after function names: those enable you to search for function definitions more easily, because function calls cannot have them.

In general, there's also the option of having the code style be applied automatically when merging a PR, or in a git hook before then, or even when saving a file: https://github.com/prettier/prettier -- So you can write the style you want when implementing a feature. It's obviously no solution for being able to comfortably read a certain style later on. But it does help with not having to adjust one's muscle memory so much, especially when switching between projects.

[galfert]

Just a note regarding spaces after function names: those enable you to search for function definitions more easily, because function calls cannot have them.

That was the sole reason for me to switch to this code style as well. I didn't like the look of it either, but the benefit to easily search for function definitions far surpasses my aesthetic preferences. And to be honest, I actually started to like the look of it as well :)

[thornjad]

Alright, I'll accept that (spaces after function names), I can get over the way it looks. I haven't run into that issue before, but I understand the utility. I have some coworkers who like doing that in Java, maybe they have a point! :) In summary, I like semistandard!

[lesion]

Done here, as said using promisify.
If you are all ok with this, we can merge in next days as armadietto/master is currently not working .

[raucao]

Awesome! 👏 🎉

As there's already a release on npm, which is advertised in the README, I think it would be useful to git-tag the commit as well, and add release notes for it.

Also, as there are zero known issues now, is it really still alpha? And should we maybe publish a 1.0.0 soon, so people can start relying on SemVer versions from then on?

[raucao]

@thornjad Did you get a chance to take it for a spin yet?

[lesion]

As there's already a release on npm, which is advertised in the README, I think it would be useful to git-tag the commit as well, and add release notes for it.

you're right.

Also, as there are zero known issues now, is it really still alpha? And should we maybe publish a 1.0.0 soon, so people can start relying on SemVer versions from then on?

ok with semver, to 1.0.0-beta let me finish to re-add redis support at least.

[thornjad]

I have! Sorry for the delay. That's a lot of refactoring and it all looks great, and I'm very happy to have promisify in the stack.