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

feat(render): overhauled qri render, implement RFC0011 #724

Merged
merged 4 commits into from
Apr 3, 2019
Merged

feat(render): overhauled qri render, implement RFC0011 #724

merged 4 commits into from
Apr 3, 2019

Conversation

b5
Copy link
Member

@b5 b5 commented Apr 2, 2019

OOooOOok, this PR implements RFC0011.

There are a few other changes worth pointing out:

Qri viz template syntax has changed

This PR completely breaks the existing viz template api, replacing it with the one the RFC describes. I plan to fully document that API & post it up in the next few days. The majority of it is outlined in the RFC.

limit, offset and all params are removed from render

Good riddance. Instead templates now bear the full responsibility of determining how much of the dataset body to read. This is a necessary change now that save and update both execute render by default. Speaking of which:

qri save and qri update both generate visualizations by default

Qri now stores an executed version of the template at index.html. Get the old behaviour back with --no-render.

b5 added 4 commits April 2, 2019 15:14
No longer needed. Instead templates specify which components of a dataset they need

BREAKING CHANGE:
qri render limit, offset, all parameters have been removed
@b5 b5 changed the title feat(render): overhauled qri render, implement RFC0012 feat(render): overhauled qri render, implement RFC0011 Apr 2, 2019
@b5 b5 marked this pull request as ready for review April 2, 2019 19:28
@b5 b5 requested review from dustmop and ramfox April 2, 2019 19:29
@b5 b5 added Breaking Change things that change existing API contracts. feat A code change that adds functionality labels Apr 2, 2019
Copy link
Member

@ramfox ramfox left a comment

Choose a reason for hiding this comment

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

I was having trouble installing this. Turns out dataset and starlark both import a package github.com/360EntSecGroup-Skylar/excelize. We use it in /starlib/xlsx/xlsx.go and /dataset/dsio/xlsx.go, specifically the Rows.Columns() func.

function signature in the release version of 360EntSecGroup-Skylar/excelize:
func (rows *Rows) Columns() []string

function signature at head:
func (rows *Rows) Columns() ([]string, error)

Our code assumes the function signature at head (which was merged 11 days ago), but when you go get or run make update-qri-deps, go will pull the release version. This won't matter once a new version of excelize is released, and it won't matter to anyone who is not compiling Qri, but I wanted to note the trouble I ran into!

@ramfox ramfox self-requested a review April 3, 2019 04:06
Copy link
Member

@ramfox ramfox left a comment

Choose a reason for hiding this comment

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

Looks good :)

@b5
Copy link
Member Author

b5 commented Apr 3, 2019

@ramfox your issue with xlsx is related to #716, and further proof that we need go modules real bad. I've opened #726 to get us going on a better fix for the problem you ran into

@b5 b5 merged commit 165abce into master Apr 3, 2019
@b5 b5 deleted the render branch April 3, 2019 14:34
@ghost ghost removed the in progress label Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change things that change existing API contracts. feat A code change that adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants