saadq / lynt

✨ A zero config JavaScript linter with support for Typescript, Flow, and React.
MIT License
384 stars 6 forks source link

fix not exporting tslint with project rules #31

Closed danielpza closed 5 years ago

danielpza commented 5 years ago

closes #30

danielpza commented 5 years ago

Should be good now, forgot to compile it :|

saadq commented 5 years ago

Yup, looks good!

Although now that I think about it, this would probably cause issues when you use the Node API from the package directly like:

import lynt from 'lynt'

const results = lynt([], { typescript: true }) 

The project flag wouldn't be getting set anymore by default as it only happens when you use it through the CLI now... I'm not sure what the best way to resolve this is, do you think we should just have the same if statement in both the cli.ts and index.ts files?

danielpza commented 5 years ago

I think we should put the exportConfig part in the index.ts file, and put the defaults there too, but I think it will take a little bit more of code and maybe you are more comfortable doing it, I could go further and make this if you are ok with this

saadq commented 5 years ago

Hmm, the only problem I have with that is that in my opinion I think the Node API should be a simple input/output to process.stdin/process.stdout. It shouldn't do side effects like creating files that exportConfig would do, which I think should be exclusively part of the CLI.

I think for now, we can just have the if checks in both places and maybe look into improving it later. Would you be alright with doing that?

And appreciate your patience with me about this! haha

danielpza commented 5 years ago

Yeah, the if could be in both part, but mean, the export config part is only available through the cli right now, shouldn't it be in the api too ? I don't want it to generate the tslint/eslint config every time is run, sorry if I wasn't clear

Something like this:

function lynt(paths: string | Array<string>, options: Options = {}): Results {
  if (options.typescript && !options.project && paths.length === 0) {
    options.project = '.'
  }

  if (options.exportConfig) {
    if (options.typescript) {
      const config = getTSLintConfig(options)
      writeFileSync('./tslint.json', JSON.stringify(config, null, 2))
      console.log(chalk.bold.green('\n\u2714 tslint.json generated\n'))
    } else {
      const config = getESLintConfig(options)
      const ignore = getESLintIgnores(options.ignore)
      writeFileSync('./eslintrc.json', JSON.stringify(config, null, 2))
      writeFileSync('./.eslintignore', ignore.join('\n'))
      console.log(chalk.bold.green('\n\u2714 eslintrc.json generated'))
      console.log(chalk.bold.green('\u2714 .eslintignore generated\n'))
    }
    return;
  }
  ...
}
saadq commented 5 years ago

I was thinking that exportConfig should only be part of the CLI, not the API. The fact that you're exporting the config generally means that it is for some other tool, not Lynt itself. When using Lynt itself (basically the API), there isn't really any use in exporting the config.

danielpza commented 5 years ago

I always picture the cli as a way of using the api, but ok, it really doesn't make sense using it for exporting the config, the two ifs looks good.

saadq commented 5 years ago

I always picture the cli as a way of using the api

I definitely agree with that (and that is how Lynt currently works). However, I don't think that means that the CLI can't have extra features that don't belong in the API. In my view, the API itself was just meant to lint files with the options you pass it. So as you mentioned, it probably doesn't make sense to do actions like exporting the config from the API itself.

Thanks again for your help @danielpa9708, it's very much appreciated!