[DONT_MERGE] Feature code quality (fix #4)

[minecrawler]

code restructuring.

[Download]

Just wrote some comments/questions.

Thanks for helping out! I see we have some very different code styles ha ha! But I think you are absolutely right to implement linting. It would be good to convince others that we take code quality and consistency serious. However maybe I can plead with you to make the linting config a bit more loose ha ha. :)

[Download]

What exactly is the reasoning behind this change? What does the const getPropertyNames = bring us? I know functions are hoisted, but all this code is packaged in a wrapper function anyway.

[minecrawler]

I recommend reading the airbnb guide, on which the linting is based. It is quite good for modern projects and explains its choices well. https://github.com/airbnb/javascript#functions

[Download]

I'm sorry, can you summarize? I see zero advantages atm.

[Download]

Been reading the AirBnB guide but afaict their argument is just 'hoisting is bad'.
Personally I feel hoisting is a feature that is not that hard to understand and actually has some advantages as well. But hey I also love the ternary operator ? and the ugly || defaultValue trick. I like shortcuts. Because they are short ;)

My problem with linting is that it ends up as a religious debate.

Ask yourself: why did AirBnB create their own coding standard? Are they really the first to do this? Why didn't they adopt e.g. https://standardjs.com/ or any of the dozens of other 'standards' in use?

[Download]

You might not believe it, but I actually think the original 75 lines of code were more easy to read then the new 112. That's probably a personal thing but I've noticed that I'm most productive if code is close together so as much as possible of it will fit on one screen. Having to scroll is actually hurting my productivity, which is why I will write if (..) <statement> on a single line and do more stuff like that that breaks convention.

[Download]

So I know ESLint makes us do this... change == to ===. It's one of the reasons I don't use ESLint normally. However this adds bytes to my production build. Uglify and friends can't know it's actually safe to use == here. So what are we buying from these extra bytes?

Could I convince you to configure ESLint to be loose on the triple equals rule? I really hate that rule.
Most of these ESLint changes I don't like, but I want this project to be usable by many teams so probably linting it is the way to go. But that triple equals rule... I hate it with passion.

[minecrawler]

What you trade here is usually stability vs. size. A few extra bytes don't really matter, as we have internet speeds in the megabytes/second readily available in many regions and it is a lot more likely that the your user is sloppy on their end and you end up comparing a string to a number or similar.

However, I will change the triple-equal back in places which feel safe, if that's important to you.

[Download]

However, I will change the triple-equal back in places which feel safe, if that's important to you.

It would be much appreciated,

[Download]

Can we maybe put this back at the top?

In the original source code, the public API stuff was at the very top and the further down you go, the more private/implementation specific/low-level the code gets. That was actually done on purpose. That way someone casually inspecting the source does not have to wade through a whole lot of nitty gritty low level private functions before he gets to the ones he is used to using in his program.

[minecrawler]

Well, you make use of JS function hoisting here, which in general is contra-intuitive. Logically, you first declare and define, then use something, however, putting the export and high-level functions to the top is the other way around.

I haven't seen a library using that kind of sorting, but if you want it that way, I can comply.

[Download]

you make use of JS function hoisting here.

Correct. I actually like that feature, because it allows me to put what's important at the top. In fact I think that's what hoisting was meant to achieve.

I haven't seen a library using that kind of sorting

Actually it's pretty common in many languages. Pascal comes to mind. Otherwise you are forced to put the low level bits at the top.

[Download]

I've been thinking some more about this.

In a perfect world, index.js would contain just some imports. So import mix.js, is.js and like.js for example. And then export them under the common namespace. As a new coder, you open index.js and see where the stuff is coming form and then open the correct file. This gives the top-down overview of the code I want to achieve. But the problem is that actually splitting up this code into 4 files gives way too much overhead. Webpack modules are powerful but they come at a price. I found this from carefully examining the packaged code.

So this technique using function hoisting is a compromise where you still get a top-down view but I can cram everything into one file to reduce overhead. Pascal did the same thing where one unit contained many functions, classes etc.

[Download]

I think it's best to just ignore versions in the feature branches. Once the feature branch is merged, we cut a new version directly from master. This prevents the feature branch having to guess at the version, or getting conflicts because the version changed from another feature being merged.

[minecrawler]

Well, you might want to think about using GitFlow, which is a lot more clear about that kind of thing. The version and final changes would be committed to a release branch, before landing in master, which always only contains a production-ready version.

[Download]

Haven't used it before but I'm open to trying it 👍

[minecrawler]

You might not believe it, but I actually think the original 75 lines of code were more easy to read then the new 112. That's probably a personal thing but I've noticed that I'm most productive if code is close together so as much as possible of it will fit on one screen. Having to scroll is actually hurting my productivity, which is why I will write if (..) on a single line and do more stuff like that that breaks convention.

Well. I found several instances of functions like this:

var foo = (() => [1,2,3].reduce((s, v) => s + v))();

and this

var bar = (() => x => [1,2,3].reduce((s, v) => s + v) + x)(baz);

That's over-engineered and unperformant and doesn't make sense. Additionally, instead of using a very simple switch-statement with a good overview, you concatenated 3 or 4 conditions with logical and ternary operators, leading to a mess I was not able to grasp at all until I carefully pulled it apart.

Opensource cuts both ways. You write source and you have to be productive, however, others who want to contribute also have to be able to read and understand the source. Since I was not able to understand the original source at all, others will feel the same way. On top of that, I have the impression that you yourself have problems understanding your own code, as you tend to over-engineer it a lot.

So, if you want to keep using your style, that's ok and I respect your decision, but I will not be able to contribute and I will not feel confident using your solution. However, if you think we can compromise, then I am onboard. I decided for the airbnb style, because I think it is safe, leads to easy to read code and is consistent with other C-like languages. If you do not like airbnb's style, we can use a different one, like JSSS.

[Download]

@minecrawler
Sorry if I came across as rigid. I'm a kinda stubborn person with strong opinions.

However I am dedicated to working as a team member on this and be open to input from you (and others willing to contribute). That said, I feel completely rewriting the code from scratch (as this PR is doing) is counter-productive.

How about this compromise:

You setup linting like in this PR, but instead of changing all the code so the linter doesn't compain, we start by making the linting (much) more loose so we have to change as little code as possible. For example I deliberately chose a semicolon-less style, so maybe we can change the linter to accomodate this.

Once this is merged, we can create a/some separate PR(s) to re-enable linting on those rules that we agree are valuable.

Opensource cuts both ways. You write source and you have to be productive, however, others who want to contribute also have to be able to read and understand the source.

Totally agree! And I should start by admitting that I am biased towards my own style. So I value your input! Hopefully we can find a middle ground that makes us both happy.

That's over-engineered and unperformant and doesn't make sense.

I am assuming you are referring to e.g.

if (args.length) factory = (org => superclass => org(args.reduce((s,m) => m.mixin(s), superclass)))(factory)

and

interface: {get:(x => () => x ? x : x = getInterface(Class.prototype))()},

I admit I may have gone a bit overboard here. :-) However I do not agree with the underperfomant aspect of your comment. In fact performance was very much on my mind when I wrote this code. A big part of perf in JS comes from a small library, which is why this code is written so terse. And in the second example, the x is a 'private' variable that is used to cache the result of the getInterface call.

However 'permature optimization is the root of all evil' they always say, so probably we should refactor this to be more readable 👍

EDIT: At the risk of making a long comment even longer, let me add this observation. This PR does a lot of stuff. Also, some stuff I see would be very useful to have, right away. But currently our 'debate' (lol) about coding style is standing in the way of a merge,

May I suggest you create a couple of smaller PRs:

• Add Jetbrains project to ignored folders
• Package lock for NPM
• Setup loose linting
• Create PR(s) for linting rules

I can then quickly merge them.

I think once this is done, you should become collaborator of this project, so you don't have to wait around for me to merge stuff. 👍

[minecrawler]

Thank you for your understanding. I guess it is important that we agree on the changes and make a compromise for the best future of the project :)

So, let's close this PR and make smaller ones. I will shift further discussion on the mentioned topics to the appropriate locations

[Download]

Great!