Skip to content

Adding __delitem__ to dict #215

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Jan 14, 2023
Merged

Adding __delitem__ to dict #215

merged 2 commits into from
Jan 14, 2023

Conversation

kellrott
Copy link
Contributor

Enables the 'del' command to delete items from a dict

@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Base: 74.35% // Head: 74.38% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (c89f95c) compared to base (c60d425).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
+ Coverage   74.35%   74.38%   +0.03%     
==========================================
  Files          76       76              
  Lines       12617    12625       +8     
==========================================
+ Hits         9381     9391      +10     
+ Misses       2563     2562       -1     
+ Partials      673      672       -1     
Impacted Files Coverage Δ
py/dict.go 79.50% <100.00%> (+1.43%) ⬆️
vm/eval.go 79.53% <0.00%> (+0.18%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

thanks for the PR.

I have 2 comments.
see below.

py/dict.go Outdated
Comment on lines 193 to 201
str, ok := key.(String)
if ok {
_, ok := d[string(str)]
if ok {
delete(d, string(str))
return None, nil
}
}
return nil, ExceptionNewf(KeyError, "%v", key)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
str, ok := key.(String)
if ok {
_, ok := d[string(str)]
if ok {
delete(d, string(str))
return None, nil
}
}
return nil, ExceptionNewf(KeyError, "%v", key)
str, ok := key.(String)
if !ok {
return nil, ExceptionNewf(KeyError, "%v", key)
}
_, ok = d[string(str)]
if !ok {
return nil, ExceptionNewf(KeyError, "%v", key)
}
delete(d, string(str))
return None, nil

yes, it's longer, but only in the vertical dimension (however, it's less indented)

del a["hello"]
assert not a.__contains__('hello')
assert a.__contains__('hi')

Copy link
Member

Choose a reason for hiding this comment

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

perhaps add a couple of cases that exercize the 2 ExceptionNewf branches ?

Copy link
Member

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

LGTM

(thanks again)

@sbinet sbinet merged commit 0cc4032 into go-python:main Jan 14, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants