search

LinkedDataFragments / Client.js

[DEPRECATED] A JavaScript client for Triple Pattern Fragments interfaces.
http://linkeddatafragments.org/
Other
92 stars 35 forks source link

Cleaned Feature more filter functions revised (#23) #30

Closed mielvds closed 4 years ago

mielvds commented 7 years ago

This is a cleaned version of #23

RubenVerborgh commented 7 years ago

@mielvds I reviewed this and would merge as feature-more-filters. Can you confirm?

PS Threw out the crypto stuff for now because it didn't work.

mielvds commented 7 years ago

Fine by me, except:

RubenVerborgh commented 7 years ago

what was wrong with the crypto? (Never tested this though)

It really didn't work. Threw something like "MD5 is not a function". The API of the cryptojs module is totally different.

not too happy with the disappearance of the unimplemented functions. Everything from the spec was in there, now we have to remember which ones are missing.

I can make comments for them. (Should always be careful about size given that this goes to browsers.)

Also, without UnimplementedOperatorError, we'll have to keep explaining to people that our implementation is not spec compliant.

I'll change the default error to "Not implemented".

why no REPLACE flags?

The code assumed that String#replace had a flags parameter in JavaScript, but it doesn't. So flags would silently fail; this makes it explicit.

are the unittests complete?

I think so, will double-check.

mielvds commented 7 years ago

The code assumed that String#replace had a flags parameter in JavaScript, but it doesn't. So flags would silently fail; this makes it explicit.

Ah yes, It should have been something like replace('/'+ a +'/' + flags, b)

RubenVerborgh commented 7 years ago

Thanks a lot, merged as 4aafe0b2b510a388cdf1a102877c63bdae004262.

RubenVerborgh commented 7 years ago

Had to unmerge because of problems with the argument checking. In the old engine, we could have:

50 > 100 // true (correct)
"50" > "100" // false (incorrect)

In the new engine, we have:

50 > 100 // true (correct)
"50" > "100" // deliberately throws type error

Unfortunately, the latter case also occurs in benchmarks, and would break our compatibility with them. As such, we should not merge before this is fixed. (In general, all FedBench queries should return the correct number of results before we merge.) Code still available as feature-more-filters.

rubensworks commented 4 years ago

Thanks for the PR, but this project has now been deprecated in favor of Comunica.