Skip to content

Add Slice function for range type - #83

Merged
corona10 merged 3 commits into
go-python:masterfrom
HyeockJinKim:issue77
Sep 26, 2019
Merged

Add Slice function for range type#83
corona10 merged 3 commits into
go-python:masterfrom
HyeockJinKim:issue77

Conversation

@HyeockJinKim

Copy link
Copy Markdown
Contributor

Crate a new range object by calculating start,
stop, and step when slice is entered as argument
to the getitem function of the range

Fixes #77

Comment thread py/range.go Outdated
@codecov-io

codecov-io commented Sep 15, 2019

Copy link
Copy Markdown

Codecov Report

Merging #83 into master will increase coverage by 0.13%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   68.84%   68.98%   +0.13%     
==========================================
  Files          59       59              
  Lines       10545    10588      +43     
==========================================
+ Hits         7260     7304      +44     
+ Misses       2775     2774       -1     
  Partials      510      510
Impacted Files Coverage Δ
py/range.go 87.09% <95.83%> (+6.84%) ⬆️
py/internal.go 41.34% <0%> (+0.48%) ⬆️

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 261242c...6ee9326. Read the comment docs.

@HyeockJinKim
HyeockJinKim force-pushed the issue77 branch 2 times, most recently from 570ec8e to b39bd4e Compare September 16, 2019 03:25

@corona10 corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@corona10 corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me
Do you have any suggestion? @ncw

@corona10
corona10 requested a review from ncw September 17, 2019 16:46
@ghost

ghost commented Sep 18, 2019

Copy link
Copy Markdown

CPython (v3.7.4) has an additional type check that raises an exception on wrong types like:
TypeError: slice indices must be integers or None or have an __index__ method.
That means types having __index__ method operate on this.
For example:

class C:
    def __index__(self):
        return 1
r = range(10)
assert len(r[:C()]) == 1
# len(r[:1.1]) -> Exception
@corona10

Copy link
Copy Markdown
Collaborator

@HyeockJinKim Please rebase the PR and reflect @Tim-St 's review.

@corona10 corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please follow the comment I left.

Crate a new range object by calculating start,
stop, and step when slice is entered as argument
to the __getitem__ function of the range

Fixes go-python#77
If __index__ is defined when class is used
as index value, __index__ value is used.
@HyeockJinKim

Copy link
Copy Markdown
Contributor Author

@Tim-St @corona10
Can I get a review again?

@corona10 corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@corona10
corona10 merged commit e427daf into go-python:master Sep 26, 2019
@ghost

ghost commented Sep 26, 2019

Copy link
Copy Markdown

@HyeockJinKim
Your code for __index__ doesn't check the return type.

For example:

class C:
    def __index__(self):
       return "abc"
r[:C()]

Should raise TypeError: __index__ returned non-int (type str).

Maybe it would be good to first call __index__ using py.Call and then check error and then the type of the returned result if no error.

Also the following should throw an Exception like in CPython:
r[:1.1] # TypeError: slice indices must be integers or None or have an __index__ method

But I think that is quite much work to implement, so we could also just handle this as a new issue.

The following code results are different in CPython (sorry, I didn't find this before):

r = range(10)
r[-2::-2] # r[-2::2] is ok 
r[-2::-1]
r[:-2:-1]
r[-2:-2:-2]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants