Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Have enabling/disabling debugging not require restarting Atom #361

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dmoonfire
Copy link
Contributor

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

When the debug function is disabled, it create a () => {} function to swallow logging. However, if the debug flag is switched on, the saved log properties retain the empty function and don't start logging until Atom is restarted.

Likewise, when the debugging is disabled keeps the debug versions of the code and they continue to operate.

In addition, localStorage.debug needed to be set for debugging to work. Since we have a configuration values, the change inserts the required local storage keys for useful debug operations.

Alternate Designs

Using the current implementation works, it just requires extra steps.

Benefits

Enabling and disabling debug should control the logging immediately, this makes it a less effort and easier to work with.

Possible Drawbacks

The handle the wrapped version of the logger requires extra complexity. There might be some edge cases and it will have a small impact on startup time to load the additional module.

Applicable Issues

@dmoonfire dmoonfire force-pushed the refactor-lazy-debug-loading branch from 39df273 to 88ad30b Compare April 4, 2021 07:17
@dmoonfire
Copy link
Contributor Author

@sadick254 would you be willing to look at this and see if it works with what you were trying to implement with your item? Thank you.

@dmoonfire
Copy link
Contributor Author

dmoonfire commented Apr 4, 2021

@sadick254 related to this, I have a question. According to #356, "It should not be loaded in the production code" means it won't ever load in the release?

If that is the case, then I might need to come up with an alternative since I added debug specifically to turn on logging to debug production spell-checking errors and have users be able to copy/paste the output so I can investigate the issues that I can't duplicate myself (such as on Macs). I would consider that a "production" use.

@9994444ggg
Copy link

Thank You Back dont help me more for the moment please.

@sadick254
Copy link
Contributor

@dmoonfire I now have this on my radar. I will look into it. Thanks for your contribution. 👍

@dmoonfire
Copy link
Contributor Author

@sadick254, I've just looked at this again and I believe it will solve the problem with #372 in a more generic manner (there is a missing const in the current, but this PR pulls the constructor into a different file).

If you don't see any problems, I think it would solve a problem a number of people have had, but I'm not sure what's happened. I can also merge it myself, but I typically request one other person review it before I do that (never trust one's own code).

@dmoonfire
Copy link
Contributor Author

This also would fix #369.

@sadick254
Copy link
Contributor

@dmoonfire. Thanks for opening a PR. I will take a look into it.

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

Successfully merging this pull request may close these issues.

3 participants