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

Cleanups #103

Merged
merged 8 commits into from
Apr 18, 2018
Merged

Cleanups #103

merged 8 commits into from
Apr 18, 2018

Conversation

pascallouisperez
Copy link
Contributor

@pascallouisperez pascallouisperez commented Apr 17, 2018

This introduces three functions on Value (and one on Type), to leverage go’s dynamic dispatch rather than rely on type switches. This will make it more natural to introduce a date and enum type, and will make the changes much more go compiler directed, vs relying on code inspection to feel confident the implementation is correct.

There are more spots where we do type dispatch, and special handling, but they are mostly ref vs not-ref related, and would matter when we introduce a map[x]y type.

Each commit in this PR is isolated, and meant to be reviewed by itself.

…will make it easier and safer to add new types (e.g. date and enum).
…h manually. This will make it easier and safer to add new values (e.g. date and enum).
…spath manually. This will make it easier and safer to add new values (e.g. date and enum).
… reduce the number of arguments passes, and name them. Also attaching functionality to the context for added convenience.
…lue rather than doing explicit type dispath manually. This will make it easier and safer to add new values (e.g. date and enum).
@pascallouisperez
Copy link
Contributor Author

Included is #85.

Copy link
Contributor

@alexandrinaw alexandrinaw left a comment

Choose a reason for hiding this comment

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

👍 this is great.

return []*Worksheet{childWs}
}
if _, ok := value.(*Slice); ok {
panic("slices-of-slices are not supported yet")
Copy link
Contributor

Choose a reason for hiding this comment

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

from #85

though when we merge, we should test slices of slices!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

figured I'd leave this as open since we have #32 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, makes sense. Didn't know about that one.

@pascallouisperez pascallouisperez merged commit 5e28c3a into master Apr 18, 2018
@lgonzalez-silen lgonzalez-silen deleted the cleanups-20180418 branch June 2, 2018 14:01
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.

2 participants