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

things specified in dataset.yaml should override transforms #456

Closed
b5 opened this issue Jun 11, 2018 · 0 comments · Fixed by #461
Closed

things specified in dataset.yaml should override transforms #456

b5 opened this issue Jun 11, 2018 · 0 comments · Fixed by #461
Assignees
Labels
discussion Open-ended request for feedback fix A bug fix
Milestone

Comments

@b5
Copy link
Member

b5 commented Jun 11, 2018

So we've made transforms, dope. Transforms have sweet convenience methods like qri.set_meta("title", "this is a title"), also dope.

But this creates ambiguity. When I set a meta.title in both the transform and in dataset.yaml, what's supposed to happen? This is effectively the same thing as supplying input data on the frontend via & API call to a dataset that also includes a transform.

I'd like to vote for things specified in dataset.yaml overriding everything. To me this places a greater burden on those writing transformations (who now need to think about weather or not a user has provided field foo already). But at least in that case they conditional statements to work around this stuff.

What's important to me here is to prioritize declarative statements put forth by the end user. If I explicitly add meta.title: "my dataset", I don't think the transform should get to override me. On the other hand, if I provide nothing and the transform auto-sets a title for me, I'm delighted. If this irritates me, I have a way to modify the output without knowing how the transform works.

Holler if you think this is silly

@b5 b5 added this to the 0.4.1 milestone Jun 11, 2018
@b5 b5 self-assigned this Jun 11, 2018
@b5 b5 added discussion Open-ended request for feedback fix A bug fix ready labels Jun 11, 2018
b5 added a commit that referenced this issue Jun 12, 2018
ugh I screwed up my commits & don't have time to de-couple them. This commit also refactors our tests in the cmd package to make them more readable / sensible

closes #456
b5 added a commit that referenced this issue Jun 12, 2018
ugh I screwed up my commits & don't have time to de-couple them. This commit also refactors our tests in the cmd package to make them more readable / sensible

closes #456
@ghost ghost added in progress and removed ready labels Jun 12, 2018
b5 added a commit that referenced this issue Jun 12, 2018
ugh I screwed up my commits & don't have time to de-couple them. This commit also refactors our tests in the cmd package to make them more readable / sensible

closes #456
@b5 b5 closed this as completed in #461 Jun 12, 2018
@ghost ghost removed the in progress label Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open-ended request for feedback fix A bug fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant