Skip to content

Adding iterator method to Dict #59

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

Merged
merged 8 commits into from
Jun 3, 2019
Merged

Adding iterator method to Dict #59

merged 8 commits into from
Jun 3, 2019

Conversation

kellrott
Copy link
Contributor

Addresses #58

@codecov-io
Copy link

codecov-io commented May 17, 2019

Codecov Report

Merging #59 into master will increase coverage by 0.44%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   67.94%   68.38%   +0.44%     
==========================================
  Files          59       59              
  Lines       10378    10435      +57     
==========================================
+ Hits         7051     7136      +85     
+ Misses       2828     2794      -34     
- Partials      499      505       +6
Impacted Files Coverage Δ
py/dict.go 63.21% <100%> (+8.55%) ⬆️
py/type.go 51.54% <0%> (+0.32%) ⬆️
py/string.go 88.2% <0%> (+2.88%) ⬆️
py/list.go 25.44% <0%> (+4.14%) ⬆️
py/sequence.go 52.68% <0%> (+18.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61059b4...a39291e. Read the comment docs.

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 tackling this.

could you also add a test exercizing this new feature?
thanks!

@ncw
Copy link
Collaborator

ncw commented May 18, 2019

According to the coverage report your test doesn't hit your added code?

https://codecov.io/gh/go-python/gpython/pull/59/diff

@kellrott
Copy link
Contributor Author

The code is covered in py/tests/dict.py line 17, but the system doesn't seem to recognize that.

Copy link
Collaborator

@ncw ncw left a comment

Choose a reason for hiding this comment

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

That is looking good! Can you just sharpen the test slightly and I'll merge - thanks :-)

py/tests/dict.py Outdated
@@ -12,4 +12,8 @@
a = repr({"a":"b","c":5.5})
assert a == "{'a': 'b', 'c': 5.5}" or a == "{'c': 5.5, 'a': 'b'}"

doc="check __iter__"
a = {"a":"b","c":5.5}
assert "a" in a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly speaking this test will no longer work when we implement the __contains__ method for dict.

Do can you change it to something like

l =  list(iter(a))
assert "a" in l
assert "c" in l
assert len(l) == 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed now

@ncw
Copy link
Collaborator

ncw commented Jun 3, 2019

That looks great now thank you! I see you added an items method too :-)

Will merge now.

@ncw ncw merged commit 7f6244e into go-python:master Jun 3, 2019
@kellrott kellrott deleted the dict-iterator branch December 24, 2022 03:20
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.

4 participants