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

Changes to cocotb.log #3673

Merged
merged 2 commits into from Feb 14, 2024
Merged

Changes to cocotb.log #3673

merged 2 commits into from Feb 14, 2024

Conversation

ktbarrett
Copy link
Member

Removes the superfluous SimLog function and make the cocotb.log module private.

@ktbarrett ktbarrett requested a review from a team January 20, 2024 23:24
Copy link

codecov bot commented Jan 20, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e9f745e) 69.01% compared to head (71000ff) 68.97%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/cocotb/clock.py 50.00% 1 Missing ⚠️
src/cocotb/triggers.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3673      +/-   ##
==========================================
- Coverage   69.01%   68.97%   -0.05%     
==========================================
  Files          45       45              
  Lines        8076     8075       -1     
  Branches     2327     2327              
==========================================
- Hits         5574     5570       -4     
- Misses       1391     1394       +3     
  Partials     1111     1111              

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

@cmarqu
Copy link
Contributor

cmarqu commented Jan 21, 2024

https://github.com/cocotb/cocotb/blob/master/docs/source/rotating_logger.rst needs adaptations as well.
Ideally we would run the code as a test (like in https://github.com/cocotb/cocotb/blob/master/docs/source/quickstart.rst?plain=1#L41-L44) and only include (parts of) the file in the docs.

src/cocotb/scheduler.py Outdated Show resolved Hide resolved
@ktbarrett
Copy link
Member Author

Hmmm... okay. I guess we are providing the classes in cocotb.log as public interfaces. I guess this should stay public. Though I really don't like shadowing the module cocotb.log with the variable cocotb.log, so I might change the name to cocotb.logging instead.

@ktbarrett ktbarrett force-pushed the cocotb-log branch 2 times, most recently from 843335e to 948b1dd Compare January 22, 2024 04:11
@ktbarrett
Copy link
Member Author

ktbarrett commented Jan 22, 2024

Odd...

Apparently, importing an object from a module also does an import of that module. And Python also fixes up relative modules import cocotb.logging into relative imports import .logging which brings in the name logging. So in cocotb/__init__.py when doing from cocotb.logging import ... the logging name is overwritten which was once the standard library logging module. Very very strange behavior IMO.

@cmarqu
Copy link
Contributor

cmarqu commented Jan 22, 2024

So the least surprising way would be to call it something like cocolog?

@ktbarrett
Copy link
Member Author

Well it only matters in the implementation of this file and specifically only on the call to logging.getLogger in __init__.py. It's not an issue for the user.

@ktbarrett ktbarrett requested a review from a team January 22, 2024 22:17
@ktbarrett ktbarrett merged commit 87e85fd into cocotb:master Feb 14, 2024
25 checks passed
@ktbarrett ktbarrett deleted the cocotb-log branch February 14, 2024 18:42
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

3 participants