Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no linting is available in the project #475

Open
nils-a opened this issue Nov 30, 2020 · 13 comments
Open

no linting is available in the project #475

nils-a opened this issue Nov 30, 2020 · 13 comments

Comments

@nils-a
Copy link
Member

nils-a commented Nov 30, 2020

The project has no linting, currently
#464 removed the reference to tslint (though it is not mentioned in the commit)

While it might have been better to add a lint script in package.json instead of removing tsling, the recommendation is eslint, now.

Having a linter would be a good start towards a common style (and also to avoid common mistakes).

@pascalberger
Copy link
Member

There would also be a ESLint issue provider for Cake.Issues if you want to have it included in the build process (easiest way probably by adding Cake.Issues.Recipe to Cake.VsCode.Recipe, like for Cake.Recipe)

nils-a added a commit to nils-a/cake-vscode that referenced this issue Nov 30, 2020
using a standard configuration,
which probably should be tailored a bit.
nils-a added a commit to nils-a/cake-vscode that referenced this issue Nov 30, 2020
mostly that is correcting var/let to const...
@nils-a
Copy link
Member Author

nils-a commented Nov 30, 2020

@pascalberger that sounds like a good idea. Additionally we should add the linting, as well as running the tests (and probably depcheck, too) to the ci build. Sounds like a new issue. @gep13 What do you think?

@pascalberger
Copy link
Member

pascalberger commented Dec 1, 2020

I created cake-contrib/Cake.VsCode.Recipe#12 for adding Cake.Issues.Recipe to Cake.VsCode.Recipe.

The question is who is responsible for running the linter. I saw you added it as a node script with nils-a@5cdfdd1. From a recipe point of view it would be cleaner if this is all done by Cake.VsCode.Recipe so that it doesn't require specific setup in the project which it builds.

@gep13
Copy link
Member

gep13 commented Dec 1, 2020

@pascalberger agreed, if we are going to add this in (behind a toggle so that it can be switched off) it would make sense to go into Cake.VsCode.Recipe, so that other extensions using the Recipe can take advantage of it. Seems like we are already going to have to get a release of Cake.VsCode.Recipe out the door, so we could sneak this in before shipping that.

@nils-a
Copy link
Member Author

nils-a commented Dec 1, 2020

@pascalberger yes - I did that with yesterday's "this is a node project"-view of the world 😄

From a Recipe-view we would best create Cake.ESLint. I guess the thing I'm trying to say is that, having eslint installed creates scripts node_modules/.bin/eslint[|.cmd|.ps1] and we would need to decide which one to call. using npm run lint is easy and does that for us - provided the npm script lint is available.

On the other hand - eslint commandline options seem not too overwhelming so a first usable version is probably rather quickly written down.

@gep13
Copy link
Member

gep13 commented Dec 1, 2020

Seems like there is an open issue about this already: https://github.com/cake-contrib/addin-requests/issues/6

@pascalberger
Copy link
Member

pascalberger commented Dec 1, 2020

You don't necessarily need a plugin to run ESLint (or any npm commend). It's also perfectly fine to just run the process directly using any of Cakes process aliases. The question should be "Who is in control of the build orchestration" ? Is it Cake? Is it npm scripts? Having one tool responsible for orchestration calling other tools, is perfectly fine. As soon as there are different orchestrators it starts to become complicated.
For example for web projects I did pipelines where Cake was the main orchestrator, which called MsBuild and gulp.js where the later run the different web tools and linters.

@gep13
Copy link
Member

gep13 commented Dec 1, 2020

Given that there is more to the overall build process, I personally would prefer that Cake is doing the work, rather than going into npm scripts doing the work, but that is just me.

@nils-a
Copy link
Member Author

nils-a commented Dec 1, 2020

Agreed. I'll see if I can switch from package.json scripts to cake.

@nils-a
Copy link
Member Author

nils-a commented Dec 1, 2020

@gep13 @pascalberger What do you think of nils-a@cd93fb7 ?

@pascalberger
Copy link
Member

@gep13 @pascalberger What do you think of nils-a@cd93fb7 ?

The question is if running linters and tests is something which we expect from the recipe script or is it something which every build script needs to implement itself? At a first glance I would say this is something which should be possible to add to Cake.VsCode.Recipe.

@gep13
Copy link
Member

gep13 commented Dec 2, 2020

@nils-a as a first pass, I think the tasks make a lot of sense, but I also agree with @pascalberger it would be good to get these added into the Recipe, so that other users don't also have to implement these steps, but rather simply have a parameter they can put in their recipe.cake file to turn it off if they don't want it.

@nils-a
Copy link
Member Author

nils-a commented Aug 7, 2021

Related to (and partially duplicates) #78

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants