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.

Sign up for GitHub

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

Jump to bottom

Added Datapackage class #25

Merged
merged 5 commits into from Dec 21, 2016
Merged

Added Datapackage class #25

merged 5 commits into from Dec 21, 2016

Conversation

dumyan
Copy link
Contributor

@dumyan dumyan commented Dec 19, 2016 •
edited by roll

fixes #11 (waffle.io supported form)

@roll please review

@dumyan dumyan added the review label Dec 19, 2016
@dumyan dumyan requested a review from roll December 19, 2016 10:38
@dumyan dumyan self-assigned this Dec 19, 2016
@roll
Copy link
Member

roll commented Dec 19, 2016

@dumyan
please rebase (or recreate PR) to remove profiles PR from it

@dumyan
Copy link
Contributor Author

dumyan commented Dec 19, 2016

@roll
Sorry for the commits baggage, I got it sorted now.

@roll roll changed the title #11/datapackage class Added Datapackage class Dec 20, 2016
roll
roll requested changes Dec 20, 2016
View reviewed changes
Copy link
Member

@roll roll left a comment •
edited

Choose a reason for hiding this comment

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

I've added a few comments but main thing is what we was discussing in Slack and what API project in #11 says - all DataPackage methods should be sync except the constructor. It's possible because we have sync methods on Profiles class. So we need to use datapackage.Profiles.validate here not async datapackage.validate. We need to instantiate Profiles class inside the async DataPackage constructor.

Usage example (from Slack):

dp = await DataPackage()
dp.descriptor
dp.add_resource()
dp.update()
// etc

For now we need to await calls of methods like update or add_resource.

src/datapackage.js
/**
* Create a Promise that will resolve with DataPackage instance.
*
* @param {Object} descriptor - a datapackage descriptor
Copy link
Member

@roll roll Dec 20, 2016 •
edited

Choose a reason for hiding this comment

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

object and also url string?

src/datapackage.js
let mergedDescriptors

return new Promise((resolve, reject) => {
this._loadDescriptor(newDescriptor).then(theDescriptor => {
Copy link
Member

Choose a reason for hiding this comment

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

we don't need to support url for newDescriptor only {Object}

@dumyan
Copy link
Contributor Author

dumyan commented Dec 21, 2016 •
edited

@roll

Please review, I have made some other changes as well: simplified code, improved docstrings and testing

ps
I now see that I have made a typo in the commit message Make update and addResource async (should be sync). Can we just correct this when squashing the commits?

roll
roll approved these changes Dec 21, 2016
View reviewed changes
Copy link
Member

@roll roll left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

PS.
We will squash into one commit so it's ok.

@roll roll merged commit 8f33faf into frictionlessdata:update Dec 21, 2016
@roll roll removed the review label Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roll roll roll approved these changes

Assignees

@dumyan dumyan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dumyan @roll