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

[REQ] add /* eslint-disable */ to JS/TypeScript files #3814

Closed
ash-r1 opened this issue Sep 1, 2019 · 17 comments
Closed

[REQ] add /* eslint-disable */ to JS/TypeScript files #3814

ash-r1 opened this issue Sep 1, 2019 · 17 comments

Comments

@ash-r1
Copy link

ash-r1 commented Sep 1, 2019

Is your feature request related to a problem? Please describe.

I'm using open-api-generator with create-react-app / typescript.

I generated dead simple API by below command:
openapi-generator generate -i openapi.yml -g typescript-axios -o ./src/api-client

Then, executed yarn start command, It shows warning:

Compiled with warnings.

./src/api-client/base.ts
  Line 17:  'AxiosPromise' is defined but never used  @typescript-eslint/no-unused-vars

./src/api-client/api.ts
  Line 19:  'COLLECTION_FORMATS' is defined but never used  @typescript-eslint/no-unused-vars

Search for the keywords to learn more about each warning.
To ignore, add // eslint-disable-next-line to the line before.

I feel these generated codes shouldn't be lint-ed.

Describe the solution you'd like

Just add /* eslint-disable */ to top of the generated .js/.ts files.

Additional context

Currently, Create React App(CRA) doesn't follow .eslintignore.
facebook/create-react-app#2339

CRA & eslint is one of the most popular tools to develop JS/TS frontend.
I feel this feature is reasonable.

@auto-labeler
Copy link

auto-labeler bot commented Sep 1, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@bodograumann
Copy link
Contributor

I don’t think this is necessary or helpful. You are responsible for the linting you are doing, @AshSuzuki. If you feel that the linting done on the generated code is to much, just add your own .eslintrc.js in the src/api-client directory and disable the respective rules.

@ash-r1
Copy link
Author

ash-r1 commented Sep 6, 2019

I think it's helpful, but unnecessary.

@bodograumann, As I mentioned in Additional context, CRA doesn't follow .eslintrc.js.

@bodograumann
Copy link
Contributor

@AshSuzuki You talked about .eslintignore, which is something different.

Another solution would be to put the generated api client into a separate npm package. Then it would be included in node_modules of your main application and you can setup your own linting for the api. This will also make the api client code more easily reusable.

@mihai-dinculescu
Copy link

mihai-dinculescu commented Sep 24, 2019

I would really like this to be added for the very same reason mentioned by @AshSuzuki .

The files in question already have // tslint:disable at the top. But tslint is no longer the preferred tool for linting TypeScript, eslint is.

I will be more than happy to submit a PR.

@ash-r1
Copy link
Author

ash-r1 commented Sep 25, 2019

@bodograumann

Yeah, I know there's several options to prevent from linting.
But I feel linting should be disabled by default.

(Sorry about late reply)

@mihai-dinculescu

Thanks. I didn't realized these files already has // tslint:disable.
As current implementation produces it, this request would be reasonable.

@mikewheaton
Copy link

It looks like #4110 made this change, so now we're just waiting for the next release to include it.

@macjohnny
Copy link
Member

Fixed in #4110

@tocosastalo
Copy link

tocosastalo commented Oct 16, 2019

For me, // eslint-disable is not enough to hide @typescript-eslint/no-unused-vars warning. I still have dozens of warnings like:

 Line 93:5:  'LoginDtoFromJSON' is defined but never used        @typescript-eslint/no-unused-vars

I would need to write /* eslint-disable */ or more specifically /* eslint @typescript-eslint/no-unused-vars: 0 */ at the beginning of generated files apis, models and runtime.

@bodograumann
Copy link
Contributor

@macjohnny
Copy link
Member

Do you want to file a PR to change this?

@mikewheaton
Copy link

Can I help with this? I'm new to the project but I'm happy to make a PR if someone points me in the right direction. I'm guessing I just change the same .mustache files that #4110 did and then run some command to generate all of the rest?

@macjohnny
Copy link
Member

@mikeewheaton yes, you need to change the mustache files and run „mvn clean package“, then run bin/typescript-axios-petstore-all.sh and
commit the mustache files plus the files changed in /samples.

@mikewheaton
Copy link

@bodograumann's comment on 802 makes a good point about how adding /* eslint-disable */ would prevent linting for everyone, even those who want it.

To me, this is really a problem with Create React App's configuration making it difficult to ignore folders. I'm going to try the solution in #2339 again. 🤞

@rubeonline
Copy link

@macjohnny it is only fixed for typescript-fetch and not for typescript-axios. Why only for fetch?

@macjohnny
Copy link
Member

it is only fixed for typescript-fetch and not for typescript-axios. Why only for fetch?

@rubeonline feel free to file a PR, contributions are welcome.

@jakub-astrahit
Copy link

@macjohnny What do I need to change to add this for typescript-axios as well?

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

No branches or pull requests

8 participants