Skip to content
This repository has been archived by the owner on Apr 16, 2021. It is now read-only.

Add CI and unit tests #29

Merged
merged 20 commits into from
Jul 8, 2020
Merged

Add CI and unit tests #29

merged 20 commits into from
Jul 8, 2020

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Jun 18, 2020

No description provided.

@mrcnski mrcnski requested a review from kwypchlo June 18, 2020 16:07
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/upload.js Outdated Show resolved Hide resolved
src/upload.js Outdated Show resolved Hide resolved
testdata/dir1/file3.jpeg Outdated Show resolved Hide resolved
src/test.js Outdated Show resolved Hide resolved
src/test.js Outdated Show resolved Hide resolved
src/test.js Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
.github/workflows/pr.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@kwypchlo kwypchlo left a comment

Choose a reason for hiding this comment

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

All methods and functions in javascript that are not constructors should be named using camel case and not pascal case - this is the industry standard. If we are refactoring this SDK then it might be a good idea to change those sooner than later.

.github/workflows/pr.yml Outdated Show resolved Hide resolved
src/download.js Outdated
@@ -24,4 +30,4 @@ function DownloadFile(path, skylink, opts) {
});
}

module.exports = { DownloadFile };
module.exports = { DefaultDownloadOptions, DownloadFile };
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to export DefaultDownloadOptions - they will be merged in if customOptions are specified.

Copy link
Contributor Author

@mrcnski mrcnski Jun 25, 2020

Choose a reason for hiding this comment

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

We are already exporting these (I'm just changing how we export it for consistency, there is apparently more than one way in JS...) so removing it would be a breaking change. Also, I think it's worthwhile for these to be exported so that developers can access the defaults if they want. If you disagree, write an issue on the skynet-docs repo so we remember to apply the change to all the SDKs.

Copy link
Contributor

Choose a reason for hiding this comment

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

These variables should follow camel case convention then.

src/upload.js Outdated
})
.catch((error) => {
reject(error);
});
});
}

module.exports = { UploadFile, UploadDirectory };
module.exports = { DefaultUploadOptions, UploadFile, UploadDirectory };
Copy link
Contributor

Choose a reason for hiding this comment

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

same as with DefaultDownloadOptions, we don't need to expose DefaultUploadOptions since they will be merged in with the customOptions anyway

.gitignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mrcnski mrcnski requested a review from kwypchlo June 25, 2020 09:43
package.json Outdated Show resolved Hide resolved
src/download.test.js Outdated Show resolved Hide resolved
src/download.js Outdated
@@ -24,4 +30,4 @@ function DownloadFile(path, skylink, opts) {
});
}

module.exports = { DownloadFile };
module.exports = { DefaultDownloadOptions, DownloadFile };
Copy link
Contributor

Choose a reason for hiding this comment

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

These variables should follow camel case convention then.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/download.test.js Outdated Show resolved Hide resolved
src/upload.test.js Outdated Show resolved Hide resolved
src/upload.test.js Outdated Show resolved Hide resolved
src/upload.test.js Outdated Show resolved Hide resolved
src/upload.test.js Outdated Show resolved Hide resolved
src/upload.js Show resolved Hide resolved
@mrcnski mrcnski requested a review from kwypchlo July 8, 2020 10:59
@kwypchlo kwypchlo merged commit 128ae3c into master Jul 8, 2020
@kwypchlo kwypchlo deleted the add-ci branch July 8, 2020 11:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants