-
Notifications
You must be signed in to change notification settings - Fork 15
v0.3.0-wip #10
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
v0.3.0-wip #10
Conversation
index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should load it this way:
, N3Util = require("n3").Util;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that if I do it this way browserify will only bundle up this one file instead of all the n3... haven't really checked if I made correct assumption :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you say about browserify is true; however, N3.js only commits to the interface offered by require('n3').
This means that, in theory, the file N3Util.js could be moved or replaced, whereas N3.Util remains.
So I leave it to you to decide this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RubenVerborgh that makes sense!
If we can assume that you won't make such changes without bumping MINOR version of your package, leaving it this way seems pretty safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, actually I could make such changes as I don't officially support the n3/lib/N3Util API, but that would be highly unlikely, so don't worry 😄
However, would you care to add a small comment to that line so it does not get copied by others inadvertently?
|
It's 👍 for me. Please fix that require and we can merge. I'll release this as 0.3.0. |
index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RubenVerborgh good point with adding comment! i even did it just before you explicitly requested it 😄
|
👍 for me. You can merge here or in a v0.3.0-wip thing. |
|
I've updated levelgraph to 0.7.0 and happily moved from " to ' 👯 |
failed to update level-test!
|
@mcollina I had search_spec failing when trying to update level-test, maybe you could take a look at it? |
|
It triggers a bug in SortJoinStream that I introduced by the refactoring in 0.7.0, I'm looking at it. |
|
With |
|
I think we can merge this one & release 0.3.0 after you resolve this issue which came up with level-test 😄 |
|
Ahum, I need to release levelgraph 0.7.1 first with the fix for this issue. I have to write a couple more unit tests for LevelGraph and redo the fix there. Hopefully tomorrow! |
|
no rush, i'll just start now looking at getting this extension to work in a web browser! |
|
it should be fixed in levelgraph/levelgraph#51, please confirm! |
|
ready for me! can you just check out 731a027 before merging and releasing new minor version? |
"to'