react-ui-org / react-ui

React UI is a themeable and performant UI library for React apps.
https://react-ui.io
MIT License
21 stars 7 forks source link

Fix forgotten console log in dev mode #533

Closed joeyyy09 closed 7 months ago

joeyyy09 commented 7 months ago

PR Description:

Changes Made: Removed the console.log statement from the codebase to improve code cleanliness and prevent unnecessary logging. Additionally, added a linting rule to disallow console.log statements unless explicitly allowed by an eslint-disable statement.

Reasoning: Removing console.log statements helps maintain a clean codebase and prevents clutter in the console output. By enforcing a linting rule to disallow console.log statements, we ensure consistency in code quality and adherence to best practices.

Impact:

Enhances code readability and maintainability by removing unnecessary logging statements. Helps prevent accidental logging of sensitive information in production environments. Encourages developers to use more robust debugging techniques, such as using breakpoints or logging to a dedicated debugging tool. Related Issue: This PR addresses issue #532. Link to issue

mbohal commented 7 months ago

Thank you for the PR.

In regards of https://github.com/react-ui-org/react-ui/pull/533/commits/48697e4118f970f8cee8a2ddabcc2155c2fb13c6

  1. I don't think there was any change in import/prefer-default-export yet it is mentioned in the commit message.
  2. Why do we allow console.error? Maybe we should forbid that too since we disable console.warn as well.

In regards of https://github.com/react-ui-org/react-ui/pull/533/commits/62d06cb287217750439eb0cb8d25baa7482ab853

The commit message in inaccurate. I think plain: "Remove forgotten console.log()" would be sufficient.

Also please see: https://github.com/react-ui-org/react-ui/blob/master/CONTRIBUTING.md#git-workflow. Specifically:

I added tag to the PR so it will be included in the correct section in Changelog upon release. Normally this is detected from branch name, but for that to work the name would have to be bugfix/

joeyyy09 commented 7 months ago

Hey, I've disabled all sorts of console log messages including the console.error one which i previously allowed. Regarding the commit messages, do you want me to edit the previous ones or is it okay if it is as it is. I can merge all the commits into one and then add a new commit message if you want me to! Btw, Thanks for helping me out

mbohal commented 7 months ago

@joeyyy09

Regarding the commit messages, do you want me to edit the previous ones or is it okay if it is as it is. I can merge all the commits into one and then add a new commit message if you want me to

Generally we strive for well documented atomic commits. I think it forces one to write better code, allows for easier code review and makes it quicker to find something in project history when needed.

This PR however is so small and simple, that I think it is fine to squash all commits into on with the commit message "Forbid console.* statements in eslint".

joeyyy09 commented 7 months ago

Squashed all commits into a single commit!

mbohal commented 7 months ago

@joeyyy09

The tests are failing:

image

Could you please add // eslint-disable-next-line no-console to the relevant lines in src/components/_helpers/transferProps.js?

The tests can be run locally by npm test and npm run lint.

Thank you very much, your effort is greatly appreciated. Sorry I did not mention it before, but I only realized when I run the tests on GH.

joeyyy09 commented 7 months ago

All the tests have been passed as well as the linting errors are resolved, PFA the screenshot regarding the same.

image

mbohal commented 7 months ago

@joeyyy09 looks good!

As a last thing, could you please squash all the commits together? I tried to do it when merging, but GH dopes not seem to allow it here.

joeyyy09 commented 7 months ago

Yeah, done! Thanks for being so patient out with this!

mbohal commented 7 months ago

@joeyyy09

Thanks for being so patient out with this!

This goes both ways. I was afraid I was being to picky.

Thanks again and if you feel like submitting any more PRs we would be very grateful.