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

chore: add kataw config, linter options and custom rules #173

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KFlash
Copy link
Contributor

@KFlash KFlash commented Aug 2, 2021

This PR deal with a 'kataw.json' in the root folder.

It fetch the json file and process the data.

It then load lint rules - including custom rules - in the same way as ESLint.

It also fetch the 'minify' and 'prettify' options.

In the end it return a config file aka ESLint.

@KFlash
Copy link
Contributor Author

KFlash commented Aug 2, 2021

@aladdin-add Can you finish this PR? It's 99% complete, but I'm not 100% confident with ESLint internals.
At least you got your custom rules now.

Need to optimize and tweak the configuration

Linter rules default folder

/scr/linter/rules

Default config rules

kataw:recommended is located here:

/scr/configuration/config

@KFlash KFlash requested a review from aladdin-add August 2, 2021 12:24
@kataw kataw deleted a comment from lgtm-com bot Aug 2, 2021
@aladdin-add
Copy link
Collaborator

aladdin-add commented Aug 3, 2021

I was writting a cli for kataw in JS(~50%, not pushed yet). but this one seems has a better completeness. :-( 

will look later. :)

@aladdin-add
Copy link
Collaborator

the main reason to use JS - dogfooding. :)

@KFlash
Copy link
Contributor Author

KFlash commented Aug 3, 2021

It's not a CLI, but loading config file and linter rules. The performance is horrible. It take near 157 ms to load while a simple parser load take 9 ms.
So you need to improve performance and get rid of all try / catch I put in there

@KFlash
Copy link
Contributor Author

KFlash commented Aug 3, 2021

Ideal if this could be re-written to use some kind of parallel processing and lazy loading, but I haven't used many of the NodejS I/O methods for years so I'm outdated :)

@kataw kataw deleted a comment from lgtm-com bot Aug 4, 2021
@KFlash
Copy link
Contributor Author

KFlash commented Aug 8, 2021

@aladdin-add friendly ping

@aladdin-add
Copy link
Collaborator

seems the code has a few i/o related code; it will block users run it on non-node.js env (e.g. browsers).

what do you think put it in another package(build on the top of kataw core), so we can focus the public API in this one?

@KFlash
Copy link
Contributor Author

KFlash commented Aug 11, 2021

The code is to get the config file. Will it not be an issue if we put it in another package to get the linter rules that the compiler needs?

What kind of i/o blockers? It only load a config file, if linting is enabled it grab the rules and return.

This is only for CLI, and that is NodeJS. Not browsers

const config = getconfigFromCli() // the code in this PR

compiler(files, conig);

@KFlash
Copy link
Contributor Author

KFlash commented Aug 11, 2021

I'm less active now do to RL and I'm also experimenting with incremental parsing that will be part of the CLI

@aladdin-add
Copy link
Collaborator

aladdin-add commented Aug 11, 2021

well, another option is to add an entry "bin" to load the config. But let's do not do io related op in "src/index.ts".

it is the same as eslint doing: https://github.com/eslint/eslint/blob/3b6cd8934b3640ffb6fa49b471babf07f0ad769a/package.json#L6-L9

@KFlash
Copy link
Contributor Author

KFlash commented Aug 11, 2021

Ok. Can you remove from "index.ts" and move to a more propriate place and load from binary as in ESLint?

@KFlash
Copy link
Contributor Author

KFlash commented Aug 14, 2021

@aladdin-add any progress on this? I managed to get incremental parsing working. Need to add an CLI option for it.

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

Successfully merging this pull request may close these issues.

None yet

2 participants