Skip to content

Bug/support nquads#148

Closed
ivikash wants to merge 8 commits into
digitalbazaar:masterfrom
ivikash:bug/support_nquads
Closed

Bug/support nquads#148
ivikash wants to merge 8 commits into
digitalbazaar:masterfrom
ivikash:bug/support_nquads

Conversation

@ivikash

@ivikash ivikash commented Aug 3, 2016

Copy link
Copy Markdown

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?

Comment thread js/jsonld.js Outdated

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

@dlongley dlongley Aug 3, 2016

Copy link
Copy Markdown
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 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

ivikash commented Aug 3, 2016

Copy link
Copy Markdown
Author

@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

ivikash commented Aug 3, 2016

Copy link
Copy Markdown
Author

And why these travis tests are failing?

Comment thread js/jsonld.js Outdated
var opts = _clone(options);

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@davidlehn

Copy link
Copy Markdown
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