Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

Windows #4

Merged
merged 8 commits into from
Nov 3, 2017
Merged

Windows #4

merged 8 commits into from
Nov 3, 2017

Conversation

richardschneider
Copy link
Contributor

Get it working on windows.

Note that I replaced the package pull-glob with glob; because it does work on windows.

This PR is dependent on ipfs/interface-datastore#12 getting released.

@richardschneider
Copy link
Contributor Author

Waiting on ipfs/interface-datastore#12 and ipfs/js-datastore-core#3 to be released.

@@ -5,7 +5,7 @@
"main": "src/index.js",
"scripts": {
"lint": "aegir-lint",
"test": "aegir-test --env node",
"test": "aegir-test --env node --timeout 20000",
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this now? Have timeouts increased with this changes? If so, we should have individual timeouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes a lot of tests were skipped. In particular the many (3 * 400) test takes a long time on my top-end laptop when sharding.

package.json Outdated
@@ -55,6 +55,7 @@
},
"contributors": [
"David Dias <daviddias.p@gmail.com>",
"Friedel Ziegelmayer <dignifiedquire@gmail.com>"
"Friedel Ziegelmayer <dignifiedquire@gmail.com>",
"Richard Schneider <makaretu@gmail.com>"
Copy link
Member

Choose a reason for hiding this comment

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

this is not needed, its added automatically on release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will reset

let pattern =
path.join(this.path, prefix, '*' + this.opts.extension)
.split(path.sep)
.join('/')
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be the inverse? Split by the key separator / and the join with the os specific path sep?

Copy link
Contributor Author

@richardschneider richardschneider Nov 3, 2017

Choose a reason for hiding this comment

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

Beg to differ. The path.join creates an os specific path. The split(path.sep) then gives an array from the os specific ath and then join('/') creates a POSIX specific path, which is what glob wants,

Copy link
Member

@dryajov dryajov Nov 3, 2017

Choose a reason for hiding this comment

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

ah glob.sync does expect POSIX / separators - makes sense 👍

@richardschneider
Copy link
Contributor Author

@dryajov do you know about flow and why the Travis CI build is failing?

@dryajov
Copy link
Member

dryajov commented Nov 3, 2017

yeah, flow will enable some stricter type checking - looking at travis now.

@richardschneider
Copy link
Contributor Author

Why is flow complaining about code in node-modules?

@dryajov
Copy link
Member

dryajov commented Nov 3, 2017

@richardschneider I believe this might help - #5. Could you try those changes in your branch?

@dryajov
Copy link
Member

dryajov commented Nov 3, 2017

@richardschneider you also need those type-flow files. I would rebase my commit onto your branch.

@dryajov
Copy link
Member

dryajov commented Nov 3, 2017

@dignifiedquire do we still want to use flow? I haven't seen it used anywhere else in the codebase and it might not worth the effort to support it at this point. I'm not sure why it broke now though, that's a good question.

@daviddias daviddias changed the base branch from master to windows-interop November 3, 2017 09:19
@daviddias daviddias merged commit 4b25195 into ipfs:windows-interop Nov 3, 2017
@dignifiedquire
Copy link
Member

Yes, unless there is a strong reason to remove it I believe it is good to keep it as it makes sure that the interface-datastore contracts are not accidentally broken.

@richardschneider richardschneider deleted the windows branch November 3, 2017 13:02
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.

4 participants