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.

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

DataFrameTypeDetector now works with nil #107

Merged
merged 1 commit into from Jul 11, 2019

Conversation

@AtharvaKhare
Copy link
Contributor

@AtharvaKhare AtharvaKhare commented Jun 22, 2019

Also added relevant tests.

Fixes #66.

Also added relevant tests
ifFound: [ false ]
ifNone: [ true ].
]

{ #category : #testing }
DataFrameTypeDetector >> canAllBeDateAndTime: aDataSeries [
[ aDataSeries do: #asDateAndTime ]
[ aDataSeries do: [ :ele | ele isNil ifFalse: [ ele asDateAndTime ]] ]
Copy link

@khinsen khinsen Jun 28, 2019

Choose a reason for hiding this comment

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

Here and in the following, I'd use ele ifNotNil: rather than ele isNil ifFalse: .

Loading

@@ -32,49 +32,51 @@ DataFrameTypeDetector >> canAllBeNumber: aDataSeries [
regex := '^[-+]?[0-9]*\.?[0-9]+([eE][-+]?[0-9]+)?$' asRegex.

^ aDataSeries
detect: [ :each | [ (regex matches: each) not ] on: Error do: [ ^ false ] ]
detect: [ :each | [ each isNil ifTrue: [ false ] ifFalse: [ (regex matches: each) not ]] on: Error do: [ ^ false ] ]
Copy link

@khinsen khinsen Jun 28, 2019

Choose a reason for hiding this comment

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

Simlarly: each ifNil: [ false ] ifNotNil: [...]

Loading

on: Error do: [ ^ false ].
^ true
]

{ #category : #testing }
DataFrameTypeDetector >> canAnyBeFloat: aDataSeries [
^ aDataSeries
detect: [ :each | each asNumber isFloat ]
detect: [ :each | each isNil ifTrue: [ false ] ifFalse: [ (each asNumber isFloat) ] ]
Copy link

@khinsen khinsen Jun 28, 2019

Choose a reason for hiding this comment

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

Simpler: each isNotNil and: [ each asNumber isFloat ].

Loading

^ aDataSeries collect: [ :each | each = 'true' ]
^ aDataSeries collect: [ :each |
each isNil
ifFalse: [ each = 'true' ] ]
Copy link

@khinsen khinsen Jun 28, 2019

Choose a reason for hiding this comment

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

Once again: ifNotNil: is clearer than isNil ifFalse:.

Loading

Copy link

@khinsen khinsen Jun 28, 2019

Choose a reason for hiding this comment

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

However, seeing all the following cases I wonder if it would be better to have a method #collectNonNils and use that instead of nil-testing in each particular use case.

Loading

Copy link
Contributor Author

@AtharvaKhare AtharvaKhare Jul 3, 2019

Choose a reason for hiding this comment

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

I'll add collectNonNils since it makes more sense. PR 102 has removeNils already so we can do dataSeries deepCopy removeNils, but one with collect would be better.

Loading

@olekscode olekscode merged commit feea7f4 into PolyMathOrg:master Jul 11, 2019
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants