Skip to content

Bug/support nquads #148

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

Closed
wants to merge 8 commits into from
Closed

Bug/support nquads #148

wants to merge 8 commits into from

Conversation

ivikash
Copy link

@ivikash ivikash commented Aug 3, 2016

Hey,

As pointed out in #141, This PR intends to support both application/nquads and application/n-quads in jsonld.js.

There are a few things, that I wanted to know

  • Should we still support application/nquads?
  • In this PR I am using options.useStandardNQuadsType and if the user passes true then only application/n-quads formatting is done. Should I make this behaviour by default? And also should I change the variable name to something more generic?

@@ -788,7 +788,7 @@ jsonld.normalize = function(input, options, callback) {
}

if('inputFormat' in options) {
if(options.inputFormat !== 'application/nquads') {
if((options.inputFormat !== 'application/nquads') || (options.inputFormat !== 'application/n-quads')) {
Copy link
Member

@dlongley dlongley Aug 3, 2016

Choose a reason for hiding this comment

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

Shouldn't this be an &&? Otherwise it would seem that this compound conditional would always be true.

That being said, it would probably be cleaner to make a shallow copy of the options and if the inputFormat is application/nquads it should just be changed early to the correct application/n-quads. Then we only need to check for that in the code later. So let's normalize to application/n-quads.

@ivikash
Copy link
Author

ivikash commented Aug 3, 2016

@dlongley So, I have made the necessary changes. Yes, indeed you were right we should just convert nquads to n-quads and infact that was something I was willing to raise from point 1. Also, now inside both if..else block we are making a shallow copy of options as opts as a result, I have moved it outside the block :)

@ivikash
Copy link
Author

ivikash commented Aug 3, 2016

And why these travis tests are failing?

var opts = _clone(options);

if('inputFormat' in opts) {
if(opts.inputFormat.indexOf('application/nquads') != -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Comparison should do:

opts.inputFormat === 'application/nquads'

Copy link
Author

Choose a reason for hiding this comment

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

Oh God! What was I thinking 😡 👊. my bad

@davidlehn davidlehn added this to the v0.5.0 milestone Jul 26, 2017
@davidlehn
Copy link
Member

Due to significant code changes, I redid this patch. Perhaps not addressing the "change the format early" issue though.
#223

@davidlehn
Copy link
Member

Fixed in 0.5.18:
eef1932

@davidlehn davidlehn closed this Jan 26, 2018
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