Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Disallow console log in ESLint #2307

Closed
Tomastomaslol opened this issue Aug 15, 2020 · 7 comments · Fixed by #2319
Closed

Disallow console log in ESLint #2307

Tomastomaslol opened this issue Aug 15, 2020 · 7 comments · Fixed by #2319
Assignees
Labels
good first issue indicates an issue is good for a first time contributor in progress indicates that issue/pull request is currently being worked on LOE - small indicates that the level of effort to complete issue is small (i.e changing the color of a button)
Milestone

Comments

@Tomastomaslol
Copy link
Contributor

Tomastomaslol commented Aug 15, 2020

🐛 Bug Report

As a followup to #2286 I thought it might be desirable to not allow any log messages in the application using an ESlint rule https://eslint.org/docs/rules/no-console

Proposed changes

  • Remove any console call from the application
  • Add a rule in the ESLint config that would make the linter fail if there is a console call in the application

Is there any good reason why console logs should be kept in the application?

@jackcmeyer
Copy link
Member

Seems reasonable to me.

Is there any good reason why console logs should be kept in the application?

If there is, we can allow console.log on a per file/line basis.

@jackcmeyer jackcmeyer added good first issue indicates an issue is good for a first time contributor help wanted indicates that an issue is open for contributions LOE - small indicates that the level of effort to complete issue is small (i.e changing the color of a button) labels Aug 18, 2020
@jackcmeyer jackcmeyer added this to the v2.0 milestone Aug 18, 2020
@tobireuen
Copy link
Contributor

I would like to work on this!

Just so we're clear: you want ANY call to console removed and forbidden, right?

@fox1t
Copy link
Member

fox1t commented Aug 18, 2020

I agree that we can whitelist console.logs where the call is really needed (spoiler alert: nowhere XD)
@tobireuen yes!

@tobireuen
Copy link
Contributor

Okay but have a look at App.tsx:24.

If I remove the console.log there, I could as well remove the catch block. Are exceptions caught on a higher level? Or how would you handle this specific case?

Please bear with me, this would be my first contribution and I want to be extra cautious.

@Fibii
Copy link
Contributor

Fibii commented Aug 19, 2020

@tobireuen, i don't think exceptions are handled on the higher level, a good way to handle that is by either informing the user with an alert or a notification with whatever error that is, or use some kind of a logger

for now you can override no-console rule for that line, unless specified otherwise by the maintainers

@tobireuen
Copy link
Contributor

@Fibii thank you, will do it as you suggested and override the rule

If someone would be so kind and assign this issue to me, I could open a pull request. 🙃

@matteovivona
Copy link
Contributor

@tobireuen done! thanks for your contribution!

@matteovivona matteovivona added in progress indicates that issue/pull request is currently being worked on and removed help wanted indicates that an issue is open for contributions labels Aug 19, 2020
tobireuen added a commit to tobireuen/hospitalrun-frontend that referenced this issue Aug 20, 2020
…no-console added

added eslint rule no-console to throw errors whenever calls to the console object are made and
subsequently removed all calls OR allowed them on file level for specific cases

fix HospitalRun#2307
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue indicates an issue is good for a first time contributor in progress indicates that issue/pull request is currently being worked on LOE - small indicates that the level of effort to complete issue is small (i.e changing the color of a button)
Projects
None yet
6 participants