LinkedDataFragments / Client.js

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

Feature more filter funtions revised #23

Closed cKlee closed 7 years ago

cKlee commented 7 years ago

We discussed this at https://github.com/LinkedDataFragments/Client.js/issues/22. Code is from the feature-more-filter-funtions branch. I just updated the deprecated N3Util.isUri to N3Util.isIRI. 381 test were passing. Although the @license comments may be wrong.

RubenVerborgh commented 7 years ago

Thanks for this effort!

I'm a bit concerned about the missing history, but I can probably rebase that; the linting would still need to be fixed, however.

RubenVerborgh commented 7 years ago

Thanks for these fixes!

mielvds commented 7 years ago

Hmm, still some issues here: see comments on the code diff

cKlee commented 7 years ago

Sorry I erased them. I can put them back in in. Should I?

RubenVerborgh commented 7 years ago

@cKlee Yes please.

RubenVerborgh commented 7 years ago

@mielvds Given that this is based on your code, do you want to take ownership of this?

I would like to do the final merge though, since I want to fix a couple of things in the branching / commit history.

RubenVerborgh commented 7 years ago

@cKlee Thanks a lot for your work, we will merge this soon.

My main concern (apart from some detailed comments) is that the code style has changed a lot, I think unnecessarily in some places. I appreciate your work on having compatibility with our linter settings, but could it be that more was changed than was necessary? Did you apply automated corrections?

RubenVerborgh commented 7 years ago

BTW First time I'm using this "review" function, so things might not fully go as expected.

mielvds commented 7 years ago

This review is still confusing yes :)

I could take ownership... but lots of the issues here are because this file was partially rewritten by you while the branch was asleep. So you'll need to do the final checks, also with the tests. I can do a cleanup in the meantime.

RubenVerborgh commented 7 years ago

@mielvds Perfect, deal!

cKlee commented 7 years ago

Let me know if I could do anything.

mielvds commented 7 years ago

Replaced by #30