fix: Windows support #7
Conversation
ghost
assigned 
src/index.js
Outdated
| @@ -136,7 +136,9 @@ class FsDatastore { | |||
| throw new Error(`Invalid extension: ${path.extname(file)}`) | |||
| } | |||
|
|
|||
| return new Key(file.slice(this.path.length, -ext.length)) | |||
| let keyname = file.slice(this.path.length, -ext.length) | |||
There was a problem hiding this comment.
Its an age thing. I come from AI/functional programming. I only use const for scalars. Consider my hand slapped.
There was a problem hiding this comment.
I'm a newbie to organisational github. How to I change a PR that you submitted? I'm use to being on the other side.
There was a problem hiding this comment.
The simpler way is to clone again.
git clone git@github.com:ipfs/js-datastore-fs.git
cd js-datastore-fs
git checkout windows-interop
# do your changes
git add <files changed>
git commit -m "message"
git push origin windows-interop
src/index.js
Outdated
| if (q.filters != null) { | ||
| filters = filters.concat(q.filters) | ||
| tasks = tasks.concat(q.filters.map(f => asyncFilter(f))) |
There was a problem hiding this comment.
Please use () for ((f) => asyncFilter(f))
There was a problem hiding this comment.
Why? I've noticed it all over the code and think its just more code, less is better.
Do you have a published coding standard?
There was a problem hiding this comment.
We do have contributing guidelines and linting rules (https://github.com/ipfs/community/blob/master/js-project-guidelines.md) but it is a bit outdated.
I do agree that less is more, but sometimes less means "I need to be familiar with some new ES6 thing" which turns out to be more. We kind of follow a slow transition path towards new ES features, we postpone anything that we consider ourselves that it is not as intuitive (yet) or that doesn't really add any urgent value.
There was a problem hiding this comment.
in this case there is a better way to write this actually:
tasks = tasks.concat(q.filters.map(asyncFilter))no need to wrap in another arrow function as far as I can telll
test/index.spec.js
Outdated
| // TODO: depends on sharding query fix | ||
| describe.skip('interface-datastore (sharding(fs))', () => { | ||
|
|
||
| describe('interface-datastore (sharding(fs))', () => { |
There was a problem hiding this comment.
Maybe not so rad, see #6, maybe the test belongs somewhere else.
There was a problem hiding this comment.
These tests do belong here, to make sure our implementation of fs passes the interface-datastore tests with sharding. Otherwise this repo can break things without ci noticing it
There was a problem hiding this comment.
okay, so I'll move the various dependencies into devDependencies.
src/index.js
Outdated
| let pattern = | ||
| path.join(this.path, prefix, '*' + this.opts.extension) | ||
| .split(path.sep) | ||
| .join('/') |
There was a problem hiding this comment.
Using a regex to do a replace should be more efficient than splitting into an array and rejoining. I.e.
// at the top of the file
const reg = new RegExp(path.sep , 'g')
path.join(..).replace(reg, '/')There was a problem hiding this comment.
Are you sure? My feeling is that regex is very expensive, compared to a few bytes (128) on the heap.
There was a problem hiding this comment.
Well split + join means you are allocating a new array and a new string, vs if you use a regex it only allocates a new string for the replacement.
There was a problem hiding this comment.
and if you are unlucky the code path for .split could end up allocating new strings for each part, so array + x string allocations for the individual parts
There was a problem hiding this comment.
Yeah but regex needs to do parsing and then build a state table.
This is about to turn into a religious war that I want to avoid. So lets kiss and makeup.
There was a problem hiding this comment.
fair enough, lets worry about it once it becomes an actual perf issue
| @@ -91,7 +91,7 @@ describe('FsDatastore', () => { | |||
| }) | |||
|
|
|||
| it('query', (done) => { | |||
| const fs = new FsStore(path.join(__dirname, 'test-repo/blocks')) | |||
| const fs = new FsStore(path.join(__dirname, 'test-repo', 'blocks')) | |||
There was a problem hiding this comment.
nice, but not needed, path.join actually takes care of the normalisation already.
path.join('a/b', 'c') === path.join('a', 'b', 'c')There was a problem hiding this comment.
Yes you are right. I was just being paranoid about slashes when trying to get it working on windows.
There was a problem hiding this comment.
Leaving out the platform-specific dividers makes it easier for everyone on platforms that don't use forward-slashes though. I think this is a good change.
|
@richardschneider there are some linting issues as well as failing tests, are you taking a look at those? |
|
@diasdavid I need your magic to make CI and flow happy @diasdavid @dryajov I need to release ipfs/js-datastore-core#6, what is the process? |
This reverts commit 71a9536.
Codecov Report
@@ Coverage Diff @@
## master #7 +/- ##
=========================================
Coverage ? 97.89%
=========================================
Files ? 1
Lines ? 95
Branches ? 0
=========================================
Hits ? 93
Misses ? 2
Partials ? 0
Continue to review full report at Codecov.
|
No description provided.