Eslint and github actions build

[rathboma]

Still a work in progress.

[rathboma]

@olifolkerd not sure if it gives you the option to, but there might be a popup on this page to enable running workflows for PRs from me. If so, if you click it it will run the build for each commit to the pr

[olifolkerd]

Alas I am currently out and about messaging on my phone. I will give it a whirl when I'm back home on Sunday.

How does it work with it building the dist files as an action. At the moment I build these on my machine, commit and push them with everything else because I like to make sure all the dists work before publishing (I've had issues in the past where a thing worked on UMD but not ESM for example)

[olifolkerd]

I'm happy to consider other options. Just not had this workflow before

[rathboma]

So here's what this does:

Linting workflow:

• Lints all files
• runs npm build. This is just to check that the build works, it's not going to commit them or do anything with them. The updated files are just discarded. It's just checking for the exit code of 0. I do this on all my commits too, just in case I forgot something, but mostly for PRs. I can make this PRs only if you prefer

Dist workflow:

• checks the PR doesn't include any changed dist files.

[olifolkerd]

So the dist one is only run on a PR not on a push to master? How is that configured out of curiosity? And how does it report an error?

There are a couple of warnings when Tabulator is built due to a couple of nessisary and non problematic circular references. Rolloup build tools handle them without issue and complete, but I just wanna check that won't break the build check

[rathboma]

It's just running your build command, it will output everything to stdout as normal, and only if rollup fails will it fail.

yes the dist one only happens on PRs, it will tell people automatically that they need to fix their branch :-).

[rathboma]

Ok, so I've done the first round of linting files. Here's what got changed:

• Lots of indentation fixes
• Lots of semicolon fixes
• Lots of spaces removed in favor of tabs
• A bunch of unreachable code removed
• re-declaration of existing variables fixed
• Removed unused variables
• Fixed some typos and reference errors
• Removed needless self assignment

There's been a couple of legit bugs it has identified (yay!)

@olifolkerd one bug I don't know how to resolve:
https://github.com/beekeeper-studio/tabulator/blob/eslint/src/js/modules/MoveRows/MoveRows.js#L73

this uses row, but row is not defined. What should it be passing here?

[rathboma]

If you want to preview what these build steps look like, check out the demo PR on my fork:

beekeeper-studio#1

You can see there's one failed task, that's the lint error from my comment above in MoveRows

[rathboma]

I get that this has created a lot of changes, and so it's hard to audit. If you wanna jump on a call to talk through this at some point, let me know

[olifolkerd]

That has picked up quite a few bits. I will check out the row issue on Sunday.

It seems to have indented all the variable definitions where there are multiple lines. I've not seen this done anywhere before. Is there any way to turn it off as I don't think it looks right to indent the definitions as they aren't in or linked to the line above?

[rathboma]

This can totally be turned off in the eslint configuration. This rule is part of the standard eslint rule set (eslint:recommended) which is probably the most widely used standard, although you can see I did change some rules to make things a little easier on the project.

[olifolkerd]

Thanks. I will have a look through the lint rules

[olifolkerd]

Hey @rathboma,

Any chance you could remove the regenerated src/dist files from the PR, i would like to experiment with the configuration without all the other updated files polluting the commit.

Also what action did you perform to process all the source files to meet the new linting guidelines?

Cheers

Oli :)

[olifolkerd]

Hey @rathboma

Any chance you caught my last message? sorry to nag, just keen to include this in the 5.2 release and need a bit of time to play around before i include it.

No probs if not, i can just copy the relevant files from the PR if i have to.

Cheers

Oli :)

[rathboma]

Hey! Just seeing it now. I'll take a look on Monday for you!

[rathboma]

So sorry I haven't been able to do this yet. I've been crazy slammed and struggling to find the time for anything. I think the scope of this PR is a little too big and it's also stopping me jumping in when I have 1/2 hour.

Here's what I'm going to do:

I'm going to make you a new PR with the eslint rules all set to 'warning', so I don't need to change all the source files. Instead of being an error, it'll just print a warning to the console.

That way you can get used to the Eslint build step and we can make individual PRs for enforcing certain styles and moving sources to be compliant (first one - making sure everything is tabs, not spaces)

Does that sound like a more manageable path forwards?

[olifolkerd]

Sounds great!

[rathboma]

See the new PR #3723