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.

Sign up for GitHub

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

Jump to bottom

triggers and utils updates #3851

Merged
merged 13 commits into from May 16, 2024
Merged

triggers and utils updates #3851

merged 13 commits into from May 16, 2024

Conversation

ktbarrett
Copy link
Member

More cleanup. I need to probably take a look at the documentation and split up util.py, but that will be a follow on PR or 2. It seemed like a good time to throw up a PR so it didn't get too big.

@ktbarrett ktbarrett requested a review from a team April 20, 2024 18:17
@ktbarrett ktbarrett marked this pull request as draft April 20, 2024 19:13
@codecov Codecov
Copy link

codecov bot commented Apr 20, 2024 •
edited

Codecov Report

Attention: Patch coverage is 92.74809% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 72.63%. Comparing base (6c42eef) to head (fa4b812).

Files Patch % Lines
src/cocotb/triggers.py 91.86% 8 Missing and 9 partials ⚠️
src/cocotb/_scheduler.py 50.00% 0 Missing and 1 partial ⚠️
src/cocotb/utils.py 97.22% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3851      +/-   ##
==========================================
- Coverage   72.67%   72.63%   -0.05%     
==========================================
  Files          49       49              
  Lines        8050     8038      -12     
  Branches     2202     2215      +13     
==========================================
- Hits         5850     5838      -12     
- Misses       1679     1694      +15     
+ Partials      521      506      -15

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

@ktbarrett ktbarrett marked this pull request as ready for review April 20, 2024 20:16
cmarqu
cmarqu reviewed Apr 20, 2024
View reviewed changes
Copy link
Contributor

@cmarqu cmarqu left a comment

Choose a reason for hiding this comment

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

More suggestions for docs outside of your actual changes, sorry.
Can't easily make a separate PR from where I am right now (bed, GH GUI :))

src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/utils.py Outdated Show resolved Hide resolved
src/cocotb/utils.py Show resolved Hide resolved
src/cocotb/utils.py Show resolved Hide resolved
@ktbarrett ktbarrett force-pushed the triggers-utils-fixes branch 5 times, most recently from f896e2c to ece57c5 Compare April 25, 2024 22:40
@ktbarrett ktbarrett requested a review from a team April 25, 2024 23:02
marlonjames
marlonjames approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@marlonjames marlonjames left a comment

Choose a reason for hiding this comment

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

Everything looks fine to me.
Do you think it would make more sense to add _LockBackCompat stuff to 1.9 and do the full switch in 2.0?

@ktbarrett ktbarrett requested a review from cmarqu April 26, 2024 12:41
@ktbarrett
Copy link
Member Author

Do you think it would make more sense to add _LockBackCompat stuff to 1.9 and do the full switch in 2.0?

Yeah probably... I'll just make the change now and put up a PR on the stable/1.9 branch.

I also need to do some newsfrags, I think.

@ktbarrett
Copy link
Member Author

@marlonjames Which changes should be put on 1.9? Just the .locked change? Probably also the .fired deprecation and also the making private of _cbhdl, _primed, and _sim_steps?

Seems like every change on master should also get a corresponding change on 1.9, giving us twice the work.

@ktbarrett ktbarrett force-pushed the triggers-utils-fixes branch 2 times, most recently from 6dc6bfc to e6e5468 Compare April 30, 2024 04:40
@ktbarrett ktbarrett mentioned this pull request Apr 30, 2024
Open
cmarqu
cmarqu approved these changes May 12, 2024
View reviewed changes
Copy link
Contributor

@cmarqu cmarqu left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long...

docs/source/newsfragments/3851.removal.rst Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
src/cocotb/triggers.py Outdated Show resolved Hide resolved
@ktbarrett
Remove usage of __slots__
55968e5 
If you have to add __dict__ to __slots__, you're missing the point.
@ktbarrett
Cleanup old-style super() calls
afdcf8d
@ktbarrett
Change description on _EdgeBase._edge_type
d033187 
class properties are deprecated in 3.11 and removed in 3.13.
@ktbarrett
Cleanup unnecessary use of fully qualified names in with_timeout
aca226b
@ktbarrett
Correct typing on Timer and get_sim_steps
d4053ff 
Python type checkers ignore the subtype relation between numbers.Real
and float because Guido doesn't like numbers. So we specifically add it
to the list.
@ktbarrett
Make implementation details of triggers private
0e21fff
@ktbarrett
Make the Event.fired attribute private
288c81f 
Use Event.is_set instead.
@ktbarrett
Make Lock.locked a method instead of an attribute to mirror asyncio
d85cfb6
@ktbarrett
Fix type checks in with_timeout
9ab3333
@ktbarrett ktbarrett merged commit ce15cd8 into cocotb:master May 16, 2024
29 checks passed
@ktbarrett ktbarrett deleted the triggers-utils-fixes branch May 16, 2024 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmarqu cmarqu cmarqu approved these changes

@marlonjames marlonjames marlonjames approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ktbarrett @cmarqu @marlonjames