Refactor the notification validation, retrieval - #209
Conversation
james-martin-jd
commented
Mar 31, 2020
- Upgraded to latest shexjs/core version
- Revamped the shexjs code so it is no longer looping over each file sequentially
- Made updates so the shex code returns a list of valid data's quads
- Updated the parsing code so it no longer re-fetches the code
- Renamed solidLDFlex to data so it is more in line with code samples of ldflex package
* Upgraded to latest shexjs/core version * Revamped the shexjs code so it is no longer looping over each file sequentially * Made updates so the shex code returns a list of valid data's quads * Updated the parsing code so it no longer re-fetches the code * Renamed solidLDFlex to data so it is more in line with code samples of ldflex package
Vinnl
left a comment
There was a problem hiding this comment.
Now that's what I call a proper commit message, thanks :)
| results.forEach(resultItem => { | ||
| if (resultItem.status === 'conformant') { | ||
| const proofStore = new N3.Store(); | ||
| Util.getProofGraph(resultItem.appinfo, proofStore, N3.DataFactory); | ||
| validQuads = [...validQuads, proofStore.getQuads()]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
It's not a biggie so no need to change it, but as a suggestion for future code: using map and filter there's no need to initialise an empty array at the top, and to keep concatenating arrays (validQuads = [...validQuads, proofStore.getQuads()]):
| results.forEach(resultItem => { | |
| if (resultItem.status === 'conformant') { | |
| const proofStore = new N3.Store(); | |
| Util.getProofGraph(resultItem.appinfo, proofStore, N3.DataFactory); | |
| validQuads = [...validQuads, proofStore.getQuads()]; | |
| } | |
| }); | |
| results | |
| .filter(resultItem => resultItem.status === 'conformant') | |
| .map(resultItem => { | |
| const proofStore = new N3.Store(); | |
| Util.getProofGraph(resultItem.appinfo, proofStore, N3.DataFactory); | |
| return proofStore.getQuads(); | |
| }); |
There was a problem hiding this comment.
I can try your suggested code out.
Originally this WAS done with .map, then .reduce. I worked with another dev (a ShEx expert) on this part of the code, but the .reduce wasn't working properly and was causing unexpected behavior.
For now, I went with a simpler, old fashioned method to get it working. I can try again with your .map and see if it works as expected now that reduce (and some of the strange internal code) has been updated and fixed.
There was a problem hiding this comment.
You could, but don't spend too much time on it. In any case it's a fine line to thread, and especially reduce can easily lead to code becoming less easy to follow. In this case it's almost a toin coss, IMHO.
* Refactored out the .foreach for a map/filter combo * Added additional commenting to make the code clearer to newcomers