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

fix: enableVerbose() not setting isVerbose correctly #140

Merged
merged 4 commits into from
Dec 22, 2023

Conversation

taras-danyliuk
Copy link
Contributor

Summary

Fix broken Meteor.enableVerbose() due to setting isVerbose in wrong scope.

Involved parts of the project

pretty much all parts :)

Targeted Meteor release version

doesn't affect communication with Meteor

Reproduction

Currently Meteor.enableVerbose() doesn't log extra info.
With this fix Meteor.enableVerbose() brings back all console.warn logs

@jankapunkt jankapunkt changed the base branch from master to dev December 21, 2023 13:29
@jankapunkt
Copy link
Member

Thank you for pointing this out! If I get it correct then you aim to make the Meteor.isVerbose calls to work when used in the other files, right? This is good but seems to cause the logs in the Meteor file to not work anymore, because there is often if (isVerbose) { using the local variable.

This is not your fault, but due to bad design (using a local variable and a property at the same time to determine if is verbose).

In order to make really all verbosity statements work I propose the following changes:

  • the local variable should be removed
  • the enableVerbose should be flagged as /** @deprecated */ but kept in, to avoid breaking things
  • all checks that use the local variable, such as if (isVerbose) { should also use if (this.isVerbose) {.

Would you mind updating your pull request accordingly?

@taras-danyliuk
Copy link
Contributor Author

I don't think it is needed to mark enableVerbose as deprecated. I didn't notice isVerbose usage as local variable, let me check and correct it

@jankapunkt jankapunkt merged commit ed9c0f6 into meteorrn:dev Dec 22, 2023
9 checks passed
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.

None yet

2 participants