Skip to content

Conversation

phi-friday
Copy link
Contributor

When I looked at where Events is used, I realized that it would be better to inherit from Enum.

@codecov
Copy link

codecov bot commented Sep 8, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.01%. Comparing base (aed6a25) to head (89f65dc).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
bayes_opt/event.py 90.90% 1 Missing ⚠️
bayes_opt/logger.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
- Coverage   96.19%   96.01%   -0.18%     
==========================================
  Files          10       10              
  Lines         867      879      +12     
==========================================
+ Hits          834      844      +10     
- Misses         33       35       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@till-m
Copy link
Member

till-m commented Sep 8, 2024

Hey @phi-friday,

You're right that enums should be used, but the current Logger setup has deeper issues beyond that. The ScreenLogger, JSONLogger, etc., aren't working well for several reasons. I’d prefer a full overhaul, moving away from the observer pattern entirely, so from my perspective this PR isn’t worth merging as a quick fix.

@phi-friday
Copy link
Contributor Author

i agree.

@phi-friday phi-friday closed this Sep 8, 2024
@phi-friday phi-friday deleted the fix-apply-enum branch September 8, 2024 09:28
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