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

Update Logger singleton creation, add test and docstring #1291

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

kremnik
Copy link
Contributor

@kremnik kremnik commented Aug 2, 2024

What has been done

With this PR, just made a few tweaks to the way we create the singleton instance of the Logger class.

  1. The check for the existence of the class instance has been moved from the global dictionary to the Logger's __new__ method.
  2. The function get_singletonish_logger() has been replaced with a simple instantiation by calling Logger().
  3. A test (in the file test_singleton.py) has been added to verify that the created instances have the same memory address.
  4. A brief docstring has been added to the Logger class.
  5. .vscode folder has been added to .gitignore.

How to test

make lint && make test

or if you want to test singleton creation only:

python -m pytest tests/test_singleton.py -s --disable-warnings

.gitignore Outdated
@@ -6,6 +6,7 @@ Pipfile
Pipfile.lock
.mypy_cache/
.idea/
.vscode/
Copy link
Owner

@serengil serengil Aug 2, 2024

Choose a reason for hiding this comment

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

this should not be added to gitignore because it stores the configuration all developers should follow.

i can update source code from different environments with same auto-linting in that way.

please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the importance of consistent configurations across different environments.

But I think keeping the .vscode folder in the repository can lead to 2 potential issues:

  1. The .vscode folder often contains individual developer settings, which might conflict with personal preferences or setups.
  2. Sometimes, .vscode/* files may include sensitive information specific to a developer's local environment.

To achieve consistency, it is possible to save essential settings in .vscode/settings.json while ignoring other files from this folder.

Let me know your thoughts!

Copy link
Owner

Choose a reason for hiding this comment

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

current .vscode in the source code does not have any sensitive information. still thinking it is important to have it.

@serengil
Copy link
Owner

serengil commented Aug 2, 2024

In previous design, we can either create a singleton logger and standalone logger. in case of creating standalone one, module is being set and this can be printed in the log messages. With this design, logger must be singleton. How can be highlighted the triggered module?

ps: not a negative comment, just to discuss

@kremnik
Copy link
Contributor Author

kremnik commented Aug 2, 2024

I haven't seen the Logger being used as a non-singleton in the current design, but it's possible I may have missed it. If having a standalone Logger with module-specific messages is needed, I can add argument checking when the Logger is instantiated using __new__ method. If it includes a "module" parameter, it creates a new standalone Logger instead of a singleton.
Let me know if this approach works for you!

@serengil
Copy link
Owner

serengil commented Aug 2, 2024

@kremnik never mind the stand alone thing. That feature is available but not being used at all.

It is good to go if you restore vscode folder.

@serengil serengil merged commit b1e8508 into serengil:master Aug 2, 2024
1 check passed
@serengil
Copy link
Owner

serengil commented Aug 2, 2024

Thank you for your contribution

@kremnik kremnik deleted the singleton branch August 2, 2024 18:18
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.

2 participants