Skip to content

Conversation

@jason-fox
Copy link
Contributor

  • split NGSI-v1, NGSI-v2 and NGSI-LD into separate files
  • Move common REST functions into restUtils.js
  • ensure that ids are URNS (adding urn:ngsi-ld: where necessary)
  • Use Batch Upsert endpoint throughout ( /ngsi-ld/v1/entityOperations/upsert/) instead of PATCH update
  • Update test expectations
  • Split NGSI v2 and NGSI-LD registrations

- split NGSI-v1, NGSI-v2 and NGSI-LD into separate files
- Move common REST functions into restUtils.js
- ensure that ids are URNS (adding urn:ngsi-ld: where necessary)
- Use Batch Upsert endpoint throughout ( `/ngsi-ld/v1/entityOperations/upsert/`) instead of PATCH update
- Update test expectations
- Split NGSI v2 and NGSI-LD registrations
@jason-fox jason-fox mentioned this pull request Feb 25, 2020
@fgalan fgalan changed the base branch from master to feature/842_ngsi_ld March 4, 2020 11:26
@fgalan
Copy link
Member

fgalan commented Mar 4, 2020

PR #843 has been merged and base branch changed to feature/842_ngsi_ld in this PR.

So I understand you should upgrade jason-fox:feature/ngsi-ld-multimeasure with telefonicaid:feature/842_ngsi_ld

);
}

//console.error(JSON.stringify(options.json, null, 4));
Copy link
Member

Choose a reason for hiding this comment

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

Leftover comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but - the multiple instances of console.error (commented out) are being used to help generate the responses for the tests.

When the final PR is processed I'll remove them all.

Copy link
Member

Choose a reason for hiding this comment

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

Noted in the accumulative PR - #849 (comment)

NTC with regards this PR

@fgalan
Copy link
Member

fgalan commented Mar 4, 2020

I think the PR is ok (except for a couple of very minor comments easy to fix)

However, regarding:

split NGSI-v1, NGSI-v2 and NGSI-LD into separate files

I like the refactor to move separate parts into -NGSI-LD.js -NGSI-v1.js and- NGSI-v2.js files in different parts. However, I wonder about the usual question: which impact could have this change into other existing PRs? Could you analyze it a little bit and provide feedback, pls?

@jason-fox
Copy link
Contributor Author

jason-fox commented Mar 4, 2020

split NGSI-v1, NGSI-v2 and NGSI-LD into separate files

This is very simple - All of the split /devices, /ngsi and /northBound files are to do with NGSI Interactions only - these are all defined by the relevant specifications, so there is zero likelihood that another PR will be amending them. It will have NO impact on the other PRs. The split has not changed the functionality of the underlying code (since none of the v1 or v2 tests has been modified) - its merely cut-paste rearrangement.

Going forward, reducing the file size means that it is even less likely that a clash will occur - besides which it is practically impossible to read a single 2000+ line file with interleaving implementations of three near duplicates of each function with only fractional differences between them. [sigh]

It also allows for:

  • the v1/v2/ld flag checking to occur once only - see jason-fox@2f2caea. (later PR)
  • a comparison to be made between the code coverage for v1, v2 and LD in coveralls.

@jason-fox
Copy link
Contributor Author

jason-fox commented Mar 4, 2020

Note that things like #835 can touch files like deviceService.js, but this is common functionality which applies to v1 v2 and LD and therefore the relevant functions (e.g. registerDevice(), prepareDeviceData() ) remain unchanged in my PR and in place in the root file.

@fgalan fgalan mentioned this pull request Mar 4, 2020
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

Ok. The rationale is sound :)

LGTM

@fgalan fgalan merged commit 8399d52 into telefonicaid:feature/842_ngsi_ld Mar 4, 2020
@jason-fox jason-fox deleted the feature/ngsi-ld-multimeasure branch March 4, 2020 19:17
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.

2 participants