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

Linting issue in no-console #11707

Merged

Conversation

thesujai
Copy link
Contributor

@thesujai thesujai commented Jan 7, 2024

Summary

Inserted no-console in rules that will warn if a conole.log is left without exception(the exception would be to disable it by using eslint-disable)

References

Fixes: #10208

Reviewers Guidance

If this is works, then i think a follow up would be to trace console.log from the code base


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: tools Internal tooling for development SIZE: very small labels Jan 7, 2024
@rtibbles
Copy link
Member

rtibbles commented Jan 8, 2024

Interestingly, the WARN still gives an error in our linting, I think maybe we can just set this to ERROR instead, as we don't seem to differentiate.

To merge this, we will need to fix the linting here. For most cases, we should use the core logging library, for an example of how it should be used, see here:

const logging = logger.getLogger(__filename);

We can use this in all cases, except for the hashi and kolibri-tools packages, where we should add the relevant ignore comment for now.

@thesujai
Copy link
Contributor Author

Interestingly, the WARN still gives an error in our linting, I think maybe we can just set this to ERROR instead, as we don't seem to differentiate.

To merge this, we will need to fix the linting here. For most cases, we should use the core logging library, for an example of how it should be used, see here:

const logging = logger.getLogger(__filename);

We can use this in all cases, except for the hashi and kolibri-tools packages, where we should add the relevant ignore comment for now.

Hello @rtibbles, Thank you for the review.
I will make the changes and let you know

@github-actions github-actions bot added DEV: renderers HTML5 apps, videos, exercises, etc. APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: small labels Jan 14, 2024
@thesujai thesujai force-pushed the eslint-rules-no-console branch from 38abe51 to e70c381 Compare January 14, 2024 13:28
@thesujai
Copy link
Contributor Author

Hello 👋🏽 @rtibbles, I made the suggested changes. Please review once
Read the docs are failing though 😕

@rtibbles
Copy link
Member

Will review - don't worry about the read the docs build, this is not caused by your work, and has been fixed on the release branch!

@rtibbles rtibbles self-assigned this Jan 15, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work, thank you!

@rtibbles rtibbles merged commit f24fdf3 into learningequality:release-v0.16.x Jan 16, 2024
@thesujai thesujai deleted the eslint-rules-no-console branch January 16, 2024 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc. DEV: tools Internal tooling for development SIZE: small SIZE: very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linting rules on console.log are missing
2 participants