Improved codebase to best practices

[joshstevens19]

Hi guys, I use your tool a lot and work for a big crytocurrency company called FunFair who use it in house. I pulled your repo to start extending some of the features but seen some files had not been updated in a year or so and some practices could be improved on.

My changes

• Make codebase more strongly typed following ts best practices
• Only imported what we need per file instead of importing the entire module from packages
• Refactored the codebase to follow best practices
• Renamed file names to be all lowercase with '-' between words, as we know linux is case sensitive and windows is not, having the files in camelcase makes it easier for windows developers to make a mistake when importing the module. This would work on windows but not on a linux computer.
• Additional changes (see pull request)

Anything you think I should not of changed please drop a comment and we can discuss.
Cheers
Josh

[juanfranblanco]

Thanks for this :). Nice refactoring, this is a rather large pull, it would have been easier to raise issues and do one at a time and discuss.

[juanfranblanco]

What I see is that you are using different linter than I am. This can be discussed. Also regarding naming of the files I work on Windows and Mac that is not an issue, but that is the current pattern, same as other extensions.

[joshstevens19]

Yeah maybe, sorry about that I just got carried away this morning. Take your time and run through it. In theory no functionality has changed all that has happened is best practices has been put in place.

[joshstevens19]

Yes Mac is insensitive so it would not matter in that regard but Linux file system is case sensitive. People seem to forget about Linux sometimes. This is why the likes of angular etc always put there files all lowercase, to avoid issues on different operating systems by easy human mistakes.

[juanfranblanco]

No worries, I know the feeling.

[juanfranblanco]

What linter are you using? I was thinking about moving to using Resharper for Typescript.

[joshstevens19]

I use tslint but with my own custom rules which I tend to not force on everyone. My refactor was more standard improvements which would not get flagged with my linter anyway.

The reason I do not like Resharper TypeScript linter is I think it can only be used on VS and not VSC. Also users have to pay for it, using a open source linter in my eyes like tslint is the way forward.

We could add some more complex rules to the current tslint in the project to force certain practices on people if you want? Thoughts?

[juanfranblanco]

After thinking I think this deserves further thoughts, I do love enforcement of only using the "methods" required not the whole library. This just inherets the standard extension initial templates (and yes the extension was created on the launch of vscode extensions, so things have evolved since).

For spacing, const vs let and other stuff like that I am not opinionated at all (as I don't use Typescript on a daily basis, I use .net hence Nethereum :) ).

But if the rules are going to change I am more interested to match vscode rules, and linter rules. (vscode GitHub project). Also like I said a code formatter like Resharper it is rather handy to do refactor everything in one go.

[juanfranblanco]

Ha ha actually you read my mind, I did not see the previous response :) :). Yes linter rules, match vscode ones will be the best, and tslint as it is used here.

Totally Resharper for Typescript you have to pay for it, but handy to reformat quickly.

[juanfranblanco]

Actually the current rules are more strict than vscode or vscode Golang

[juanfranblanco]

File naming is like this https://github.com/Microsoft/vscode/tree/master/src/vs/code/electron-main

[juanfranblanco]

Also the inclusion of vscode is standard, also it makes it easier to understand where the types come from.

See Golang https://github.com/Microsoft/vscode-go/blob/master/src/goMain.ts

[juanfranblanco]

Const vs let yes that is cool.

[juanfranblanco]

So in general..

Keep the tslint rules, they are strict enough now, you can suggest any changes. There are changes in spacing that break the rules.

Vscode can be imported as it easier to understand and main extensions use it.

No file name changes, following vscode and vscode golang format. (Already following that format, that is the reason, but as usual things might have changed :) )

Const instead of let, yep.

Need to do more checks...

[juanfranblanco]

More fs , path and general libraries also not use specific imports, vscode follows that pattern. So only specic imports for all local. (It seems to be the pattern, I believe I followed that, with a couple of exceptions). So the rule only specific imports for local as a general rule.

[joshstevens19]

Ok no worries

So i will revert:

• File name changes
• .... anything else.

If you can make a list and comment on code snippets I can revert these and make sure the PR is what you guys are after.

[juanfranblanco]

Actually it becomes unreadable in some scenarios, you want to know what you are actually referencing for general context.

[juanfranblanco]

It is actually rather hard, to point each occurrence. As the code formatting has been applied to the whole library. (Hence I asked about Resharper;) )

I would just create a full new pull request with just:

Const instead of let where you see required.

Import specific methods only for local IF it is still readable. There are probably a couple of occurrences.

No changes on spacing formatting (linter rules).

Leave the rest or let me know if something you believe is important, the guidance is vscode and vscode golang

[joshstevens19]

We should really only import what we need, it is completely best practice in JS projects.

Importing everything actually adds some negatives so I highly suggest we import what we need but cast the methods as a readable name?

Importing all of fs, path, vscode is exposing so much to the current class.

Just my view.. !

[juanfranblanco]

Well this is a electron application, so there is not really a major issue. Microsoft does it in vscode and golang and yes it is so much readable. I followed those patterns, hence I checked :). But also review yours just in case it made more sense.

Another point is those are the point of reference for extensiond, actually golang has diverted slightly which is a pain from the docs, so new features should be easily implemented. IE I don't want anybody contributing to refactor the vscode out.

[juanfranblanco]

As an example https://github.com/Microsoft/vscode/blob/master/src/vs/code/node/cliProcessMain.ts#L9

[juanfranblanco]

And another example https://github.com/Microsoft/vscode-go/blob/master/src/goMain.ts#L8

[juanfranblanco]

And again many thanks!! :)

[joshstevens19]

Ok that is fine, it doesn’t really matter deep
down! I want to keep it inline with how you want it to be.

I have some time tomorrow morning so i will reset my changes on my fork and remove the stuff we discussed above! If that means another pull request (not sure if it resolve itself when I do the changes on the branch) then I will create one.

Got a few things I want to extend and happy to start looking at some of these issues which have been raised.

Thanks for a good discussion!

[juanfranblanco]

Btw are you creating an ethereum package manager? Is that same as the one of the foundation/ pipermerrians?

[joshstevens19]

Yes I am going to get it completed by end of month (once I have time). It is going to be fully featured, logging in, updating. Kinda like NPM.

There are a few which try to do this but do not do it well, I aim to make a really useful CLI, API and UI to install and share these solidity packages.

I think something like this could really be useful for the community. Even extending it further for private packages (in-house team)

Even though anyone can get access to the code when embedded in the blockchain anyway.

This is something which I know FunFair would benefit from and other blockchain companies.

Happy for some help you want to be involved?

[juanfranblanco]

Cool probably easier to just create new pulls all together.
1 for const / let.
2 for local reference using *.
And yeah thank you on helping on issues, go to Gitter and we can chat before hand. I don't want to waste any of your time. I feel bad enough already.

[juanfranblanco]

On the epm, check on the foundation work. It will be great you coordinate with them. IMHO

[joshstevens19]

You got any links for foundation work codebase and what they do already.

Do you want me to create different pull requests for different changes in this repo?

Do not feel bad I am happy to change to what you feel is best!

[juanfranblanco]

Pulls, yes please. Bin the other.

ethereum/EIPs#190

Also go to Gitter and chat with the Solidity dev team

[joshstevens19]

Thanks for this, I am going to get all the logic written by end of play August then get in contact with them to see if they want this package under there namespace.

I have a solid idea in how this needs to work, especially getting feedback from FunFair itself 👍! Like I said welcome for help, have a look at the repo in 2 weeks. Feedback will be welcome.

Anything which you think I should include?

[juanfranblanco]

The truth is that I have just found out about FunFair I assume you are in contact with @Arachnid about this as he is the advisor of FunFair and also he is part of the Foundation.

Like I said, my only issue with this is that there is already a package management system so this deserves some level of conversation, as you will want Truffle, Populous, Zeppelin, Dapple and even Nethererum or this vs code solidity extension (and obviously other extensions to use something like this) hence there are ERCs etc.

Mainly I don't want you to encounter another "Standard" issue like this one, when there is already another standard that is used widely, already developed. So get feedback as early as possible for the community, I would start by contacting the people in that ERC and Nick if you haven't already.

By the way thanks again, I look forward to your next fresh pull with the "const / let", (if you have time, there is no rush). But I would appreciate a fresh fork / pull, so we don't clutter the history.

Thanks!!