Enforce code styles

[y-lohse]

A consistent coding style should be automatically enforced on this repo. Rough list of things to do:

• add an .editorconfig file
• add eslint and configure it so that we can keep a code-style that's close to the original C#
• add a lint task and have travis run it

[ephread]

By the way, do you use any specific style guide?

Let's take advantage of this issue to lay out all the rules that you'd like us to follow and ideally, if it's a well-known style, all the formatting rules will have already been laid out in ready to use packages.

Also, I don't believe that we should necessarily follow the formatting of the C# code. The structure has become sufficiently different so that line numbers and function order don't match. And, as long as the signatures match, it's fairly easy to recognize the similar patterns in both Javascript and C#.

We should definitely pick a style and make all the code conform to it.

[y-lohse]

So normally, I'd be in favour of just using an existing style guide and not spending much time setting up specific rules and stuff.

But while I hear your argument on the C# structure, I tend to disagree. Line numbers aren't too important, but I've found that keeping the same order and structure as much as possible is immensely helpful in the long run. When looking at a C# diff, it's a lot easier when you can just scroll along in the js/ts code, instead of searching for function names and jumping around the code.

What I had in mind was essentially taking eslint:recommended as a starting point, and then decide on a case by case basis for the rules that do break if we should change the rule or the code. i got s far as this:

"indent": [
            "error",
            2
        ],
        "linebreak-style": [
            "error",
            "unix"
        ],
        "quotes": [
            "error",
            "single"
        ],
        "semi": [
            "error",
            "always"
        ],
        "no-cond-assign": "off",
        "no-constant-condition": ["error", { "checkLoops": false }],
        "no-console": "off"

So typically no-cond-assign and no-constant-condition allow us to stay closer to the C# code, so I changed the rules. But errors about vars being re-assigned are a good point and don't actually impact legibility, so the code should be fixed on those.

As I said in the other issue, there are some rules that I actually feel somewhat strongly about — ie. not abusing const or keeping line numbers (it makes me loose the habit of using them and then I wonder why my code in other languages crash). On the other hand, I don't care at all about indentation styles or linebreaks.

I hope this gives at least a general idea of what I had in mind? I think that as part of #175 , we should keep linting related changes to a minimum, and we can iterate on that.

[ephread]

But while I hear your argument on the C# structure, I tend to disagree. Line numbers aren't too important, but I've found that keeping the same order and structure as much as possible is immensely helpful in the long run.

Don't worry, I wasn't suggesting to throw everything out the window 😉. I'll unpack my thoughts a bit better this time!

About the styling

Also, I don't believe that we should necessarily follow the formatting of the C# code.

There are a number of style inconsistencies throughout the code, which I attribute to copy/paste. For instance, sometimes we have if (foo == bar){ and sometimes we have if (foo == bar) {. Sometimes we have if( foo.isNull ) and sometimes we have if(foo.isNull).

Same goes for most of the constructs, but it's especially true for if-else. I thing we should define a set of rules on how to format them, even if it doesn't match exactly the C# original formatting (which is sometimes inconsistent too). That way, we might even make a prettier config and enjoy the benefits of automatic formatting.

When porting the changes from v8, I tried to both stick to the original formatting, while also matching the current style of the code, which was sometimes a bit disconcerting.

About the order of functions / properties

The structure has become sufficiently different so that line numbers and function order don't match.

I can illustrate the pain point with the following example: in the original C#, instance variable declarations are often following property declarations (though they sometimes seem to be dangling, like PushPopType stackPushType in Divert.cs), while in Javascript they were all declared in the constructor (language limitation).

When I said that the function order doesn't match, I meant that the constructor was usually found at the top of the class declaration, followed by getter/setters and then regular functions. Which wasn't at all the case in C#. The relative order was kept within each block of similar constructs, but the overall one wasn't. It took me a while to wrap my head around it, honestly.

Now with TypeScript, we're a bit more flexible. I believe we could scatter the type annotation of instance variable to match what is done in the C# code, but an important question is whether we should.

I totally agree that we should keep the relative order, by the way. But my concern was more about whether we should strive to match the absolute order exactly or write more usual Javascript/TypeScript (e. g. from top to bottom: variable, constructor, getter/setter, methods), while detailing a set of rules about how to organize stuff in a CONTRIBUTING.md.

---

keeping line numbers (it makes me loose the habit of using them and then I wonder why my code in other languages crash)

Did you mean semicolons?

[y-lohse]

Thanks for elaborating! You're right, i misunderstood what you meant.

About the styling

Yep, totally agree on this. And it's exactly the sort of things that I don't think is worth spending much time on — let's pick whatever eslint / tslint / prettier recommends. We can write however we want, and the robots can format it afterwards.

About the order of functions / properties

Ah yes. This was my own inconsistency with the original port, sorry for the headaches :D.

In hindsight, I think I should have tried to stick more closely to the C# version, and with typescript making that even more possible, I'm convinced we should migrate the mismatches to mirror the C# version more closely. This is also a relatively simple rule to layout for contributors, albeit it can't be automatically enforced.

If I understood correctly you're one the same boat, so we could go for that. But maybe we leave that for after the initial port to ts so we don't pack too many things in one go!

Did you mean semicolons?

Yes 🤦‍♂️ .

[y-lohse]

tslint took care of a lot of that, but I don't think it enforces anything in regards to spacing in conditions or brackets. We should check if it can be configured to do that, and if not find another tool that can do it. I'm guessing prettier supports typescript too.

[ephread]

Prettier does support TypeScript, yes!

[y-lohse]

This issue is now mostly resolved, as we have a linter and the rules are enforced through the CI. So I would say the main goal is achieved: the coding style is consistent throughout the project and contributors know what to do.

What's left to do is getting as close as possible to the default recomended typescript config. We currently have a few extra rules defined: https://github.com/y-lohse/inkjs/blob/master/.eslintrc.js#L26-L43

Next steps:

• Figure out which rules should remain off in order to keep the source in line with upstream, regardless of what typescript recommends
• Figure out which rules we should adopt and make the necessary changes
• Figure out which rules need to be disabled on a per-case basis, and use eslint-disable where appropriate instead of deactivating the rule globally