Skip to content

Conversation

@elf-pavlik
Copy link
Member

index.js Outdated
Copy link
Collaborator

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;

Ref: https://github.com/RubenVerborgh/N3.js#utility

Copy link
Member Author

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 :)

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.

Copy link
Member Author

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?

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?

@mcollina
Copy link
Collaborator

It's 👍 for me.

Please fix that require and we can merge. I'll release this as 0.3.0.
Do you plan to do some more work for the next release?

@elf-pavlik
Copy link
Member Author

after i fix directly relevant #11 and update readme with information on how we store literals you can go ahead and release 0.3.0 📦

oh, i also should add tests for fix of #4 !

index.js Outdated
Copy link
Member Author

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 😄

@elf-pavlik
Copy link
Member Author

@mcollina if you agree with keeping this way of requiring N3Util I will merge it after adding tests for #4

in case you have missed my last few exchanges about it with Ruben please just check your mail notifications since those line comments disappeared from here after my last push!

@elf-pavlik
Copy link
Member Author

ready!

just added few tests for IRIs so after merging we can also close #4

i'll prepare another PR for #11 and then we can release it as 0.3.0 📦

@ghost ghost assigned elf-pavlik Dec 22, 2013
@mcollina
Copy link
Collaborator

👍 for me. You can merge here or in a v0.3.0-wip thing.
As we are breaking the API, we can already base it on top of levelgraph v0.7, which is ready and big (levelgraph/levelgraph#42)

@elf-pavlik
Copy link
Member Author

I've updated levelgraph to 0.7.0 and happily moved from " to ' 👯

@elf-pavlik
Copy link
Member Author

@mcollina I had search_spec failing when trying to update level-test, maybe you could take a look at it?

@mcollina
Copy link
Collaborator

It triggers a bug in SortJoinStream that I introduced by the refactoring in 0.7.0, I'm looking at it.

@mcollina
Copy link
Collaborator

With { joinAlgorithm: 'simple' } works fine, so there is a regression in the fastest merge sort implementation.

@elf-pavlik
Copy link
Member Author

I think we can merge this one & release 0.3.0 after you resolve this issue which came up with level-test 😄

@mcollina
Copy link
Collaborator

Ahum, I need to release levelgraph 0.7.1 first with the fix for this issue.
It's much more complicated, but I nailed it.

I have to write a couple more unit tests for LevelGraph and redo the fix there. Hopefully tomorrow!

@elf-pavlik
Copy link
Member Author

no rush, i'll just start now looking at getting this extension to work in a web browser!

@mcollina
Copy link
Collaborator

it should be fixed in levelgraph/levelgraph#51, please confirm!

@elf-pavlik
Copy link
Member Author

ready for me! can you just check out 731a027 before merging and releasing new minor version?

mcollina added a commit that referenced this pull request Dec 30, 2013
@mcollina mcollina merged commit 8e3e8d3 into levelgraph:master Dec 30, 2013
@elf-pavlik elf-pavlik deleted the issue/datatype branch December 30, 2013 16:02
elf-pavlik pushed a commit that referenced this pull request Jan 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants