dvlsg / async-csp

CSP style channels using ES7 async/await
MIT License
317 stars 18 forks source link

Refactoring #10

Closed ivan-kleshnin closed 8 years ago

ivan-kleshnin commented 8 years ago

I'm trying to reassemble your library from scrach (for learning purposes). So far, this is what I think can be simplified.

dvlsg commented 8 years ago

Good point, that chunk definitely isn't necessary anymore.

Looks mostly good to me -- gulp lint is throwing some no-use-before-undef warnings. I know they don't actually matter, as far as the code is concerned, since the functions will definitely be defined before they are used, but I like the guarantee that I can scroll up to find a function declaration (within reason, anyways). Can you move the function declarations to be in this order?

//...
function canSlidePuts() {}
function canSlideTakes() {}
function canSlide() {}
async function _bufferedSlide() {}
//...

I'll be adding a CONTRIBUTING.md for some guidelines at some point in the future, which will include prefacing all commit comments with a type to improve readability at a glance, similar to how the AngularJS project is handled. Since I don't have it up yet, I don't really care and you can leave the messages as-is if you want, but if you'd like to follow the guidelines that will be there, you could preface your commit messages with refactor: {message}.

Some day I'll get CI connected to this project. I'll work on that when I get a chance, hopefully in the near future, especially now that we're getting some more pull requests coming in.

ivan-kleshnin commented 8 years ago

Sure, no problem.

but if you'd like to follow the guidelines that will be there, you could preface your commit messages with refactor: {message}.

I'm not sure how to do it at this point. I barely know Git. Git rebase / squash?

dvlsg commented 8 years ago

Ah, of course! I actually didn't know what squash even meant, up until a few months ago. This stackoverflow question describes a couple different ways of doing it, and this article explains the rebase version fairly well. You can also modify the commit message while you're compressing the commits down to a single commit.

If you don't want to, that's fine as well. In some cases I would consider it a bad idea (ie - if you had a commit containing a fix and a commit containing a test, I would prefer those to stay separate). Figured it might be worth learning, either way, since some projects require it for contribution. If you want to skip it, just let me know, and I'll merge as-is.

ivan-kleshnin commented 8 years ago

@dvlsg I'll try to learn how to do this. Because, yeah, it's something that may come in use soon. But it may take some time, maybe two days. In case we are not in hurry with this (I think we aren't) – I'm ok.

dvlsg commented 8 years ago

Of course -- take as much time as you need.

ivan-kleshnin commented 8 years ago

Closed in favor of #11