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

Fixing:#655 #690

Closed
wants to merge 7 commits into from
Closed

Fixing:#655 #690

wants to merge 7 commits into from

Conversation

SatyamMattoo
Copy link
Contributor

Short Description

Adding silent mode to the SanitizePolicy in order to disable all logs. Created a silent function to return null when silent mode is enabled.

Related issue

Fixes #655

QA Notes

Added tests for the silent mode testing an object and an array.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added unit tests
  • Changesets have been added (if there are production code changes)

Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

Hello again @SatyamMattoo !

Thanks for another contribution.

Have you tested this yourself be running the CLI locally? I think this will behave exactly the same as "remove", and only remove objects and arrays. Most regular log options will still be displayed.

This issue, like many others, is only roughly specced out. It's more of a reminder to me and our product management team about what work needs doing in future. It's a hard place for you to jump into without any context.

If you're keen to make more contributions, next week I can take a look and find something more suitable for you. And make sure sufficient context is set in the actual issue .

@SatyamMattoo
Copy link
Contributor Author

Hello @josephjclark,

I apologize for the oversight in my testing process. It appears that my tests only covered the cases I introduced, overlooking others. I will rectify this by reviewing and adjusting the changes accordingly.

If you believe this issue may be too complex for me, I am more than willing to redirect my efforts to other tasks better suited to me within the organization.

Thank you for your understanding and guidance.

Best regards

@josephjclark
Copy link
Collaborator

@SatyamMattoo no need to apologise!

Just remember to test the issue before you even start working on it so that you understand what you're trying to do. If you can't reproduce the problem, how would you expect to fix it?

It's not so much that the issue is complex, it's more that the solution isn't totally obvious to me. I welcome ideas - it again, you need to understand the problem first.

Always feel free to ask questions!

@SatyamMattoo
Copy link
Contributor Author

SatyamMattoo commented May 14, 2024

I believe I have a potential solution to address this issue. A straightforward approach would involve adjusting the sanitization policy to 'silent', thereby allowing the function to return without executing further actions.

const log = (name: string | undefined, level: LogFns, ...args: LogArgs) => {
    if (priority[level] >= minLevel) {
    //if sanitization policy is silent it won't log anything
      if (options.sanitize != 'silent') {
        if (options.json) {
          logJSON(name, level, ...args);
        } else {
          logString(name, level, ...args);
        }
      }
    }
  };

Now where we set When configuring the logger as follows:

const logger = createLogger('x', { level: 'debug', sanitize: 'silent' });
logger.log('abc');

The resulting output would be:

{
  level: undefined,
  namespace: undefined,
  icon: undefined,
  message: ''
}

This behavior appears to match the expected outcome, effectively disabling logging functionality.

@josephjclark
Copy link
Collaborator

Hi @SatyamMattoo,

I don't think this will help. We already have a log level of none which will do what we want (which is stop all logs).

I don't know if you saw but I left more notes in the original issue. This actually has nothing to do with sanitisation policies.

I'll have a look at this issues this morning and see if can find something more suitable for you.

@josephjclark
Copy link
Collaborator

Hi @SatyamMattoo I've updated the list of Good First Issues - perhaps you can find something in there to work on?

I've tried to fill out the requirements but I expect you will have questions - please submit them in the issue

@SatyamMattoo
Copy link
Contributor Author

I will surely check them out. Thank you so much.

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

Successfully merging this pull request may close these issues.

worker: support disabling all logs
2 participants