Skip to content

Adding split method to string class - #60

Merged
ncw merged 6 commits into
go-python:masterfrom
bmeg:string-split
May 28, 2019
Merged

Adding split method to string class#60
ncw merged 6 commits into
go-python:masterfrom
bmeg:string-split

Conversation

@kellrott

Copy link
Copy Markdown
Contributor

Added split method to str:

>>> "hello|world".split("|")
['hello', 'world']

@codecov-io

codecov-io commented May 17, 2019

Copy link
Copy Markdown

Codecov Report

Merging #60 into master will increase coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #60 +/- ##
==========================================
+ Coverage 67.94% 68.18% +0.24% 
==========================================
Files 59 59 Lines 10378 10423 +45 ==========================================
+ Hits 7051 7107 +56 + Misses 2828 2813 -15 - Partials 499 503 +4
Impacted FilesCoverage Δ
py/string.go88.2% <100%> (+2.88%)⬆️
py/type.go51.54% <0%> (+0.32%)⬆️
py/sequence.go36.55% <0%> (+2.15%)⬆️
py/list.go25.44% <0%> (+4.14%)⬆️

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...e316b2b. Read the comment docs.

@corona10
corona10 requested a review from ncwMay 18, 2019 01:43

@ncwncw 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.

I think that is an excellent start :-)

However there is quite a bit of str.split functionality missing which I've outlined - fancy adding it?

Comment threadpy/string.go Outdated
}
return &o, nil
}
return nil, fmt.Errorf("Not split by string")

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.

This should be a TypeError

>>> str.split(8)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: descriptor 'split' requires a 'str' object but received a 'int'

Comment threadpy/string.go
return &o, nil
}
return nil, fmt.Errorf("Not split by string")
}, 0, "split(sub) -> split string with sub.")

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.

The Python help is

 S.split([sep [,maxsplit]]) -> list of strings
Return a list of the words in the string S, using sep as the
delimiter string. If maxsplit is given, at most maxsplit
splits are done. If sep is not specified or is None, any
whitespace string is a separator and empty strings are removed
from the result.

Which makes me see that we are missing two things.

  • If sep isn't passed (or is None) in then we should be using strings.Fields in go terms (This doesn't have a maxfields parameter though.)
  • There is another optional parameter to specify the number of splits.

Here are some cases to consider

>>> "a,d,c".split(",")
['a', 'd', 'c']
>>> "a,d,c".split(",",1)
['a', 'd,c']
>>> " a d b ".split()
['a', 'd', 'b']
>>> " a d b ".split(None, 1)
['a', 'd b ']

Comment threadpy/tests/string.py Outdated
ncw
ncw approved these changes May 28, 2019
@ncw

ncw commented May 28, 2019

Copy link
Copy Markdown
Collaborator

That looks great now thank you and 100% diff coverage too :-)

I'll merge it now - thank you very much.

@ncw
ncw merged commit f76e37b into go-python:masterMay 28, 2019
@kellrott
kellrott deleted the string-split branch December 24, 2022 03:20
Sign up for freeto 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.

3 participants

@kellrott@codecov-io@ncw