Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Obtaining all collection members fixing issue 33 #34

Conversation

@alien-mcl
Copy link
Member

alien-mcl commented Apr 10, 2018

Please find the attached pull request fixing issue 33 which is related to obtaining all collection members.


This change is Reviewable

@tpluscode

This comment has been minimized.

Copy link
Contributor

tpluscode commented Apr 16, 2018

My only thought is to make such API not greedy, at least limited but ideally working on-demand.

Think of an infinite scroll, where the client page GETs the rel="next" when it's scrolled to the bottom at which point it would fetch the additional items.

The question is whether the API should incorporate the extra items within the same JS object or return one completely new. I'd say the latter to keep the methods pure. Something like

const collection = client.getTheResourceSomehow(/* :) */);

// the representation above would be combined with the next page
collection = client.appendNextPage(collection);

I don't think this has to be really complicated actually. If the collection is not paged (or the last page), it would jus return the same object. In other cases it would combine the member collection of both and somehow merge the view.


Initial page:

{
  "@id": "/c?page=1",
  "member": [
    { "@id": "m1" },
    { "@id": "m2" }
  ],
  "view": {
    "first": "/c?page=1",
    "previous": "/c?page=1",
    "next": "/c?page=2",
    "last": "/c?page=10"
  }
}

Next page:

{
  "@id": "/c?page=2",
  "member": [
    { "@id": "m3" },
    { "@id": "m4" }
  ],
  "view": {
    "first": "/c?page=1",
    "previous": "/c?page=2",
    "next": "/c?page=3",
    "last": "/c?page=10"
  }
}

I think the combined representation should:

  • merge members
  • keep first and last (the would be the same anyway
  • keep next of the the appended page's view
  • keep previous of the initial page's view
  • keep @id of the appended page
{
  "@id": "/c?page=2",
  "member": [
    { "@id": "m1" },
    { "@id": "m2" }
  ],
  "view": {
    "first": "/c?page=1",
    "previous": "/c?page=1",
    "next": "/c?page=3",
    "last": "/c?page=10"
  }
}
@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Apr 16, 2018

What if the initial page is not the first one? Should we revert back to the first and follow next links or proceed from that very point?
As for the non-paged collection - it's already behaving as you wrote - the initial members collection is provided without any additional requests.


Review status: 0 of 23 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@tpluscode

This comment has been minimized.

Copy link
Contributor

tpluscode commented Apr 16, 2018

This question crossed my mind. I think that in the "page-by-page" mode it should just start off from whatever state forward. Again, the infinite scroll would work that way. The UI at Nth page and the traverses through pages N+1, N+2 and so on.

The same principle can be applied in the functionality of retrieving the whole thing. Only that the client would start requesting in both directions N+1, N+2... and N-1, N-2 until there is not previous/next link respectively.

I find the latter a special case of a the "page-by-page" approach. And if the infinite scroller was horizontal (think tablet) it makes all the sense to have a function to go one bye one in both directions too. And then the "get entire collection" is just two loops and two if conditions :)


Review status: 0 of 23 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Apr 16, 2018

Unless client can control server on the number of provided members, I think getting members one-by-one is not a subject here and would need to be implemented above the generic client.
As for the bidirectional example - that would imply an additional parameter or separate method for it, i.e.:

let batch = client
    .getResource("http://temp.uri/api/collection")
    .hypermedia
    .collections.first()
    .getAllMembers({ backward: false })
//...
batch = batch.next();

Review status: 0 of 23 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Apr 16, 2018

Actually the snipped can be shortened to the one below:

let batch = client
    .getResource("http://temp.uri/api/collection")
    .getAllMembers({ backward: false })
//...
batch = batch.next();

Review status: 0 of 23 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@tpluscode

This comment has been minimized.

Copy link
Contributor

tpluscode commented Apr 16, 2018

I think getting members one-by-one is not a subject here

I did mean one by one page.


Review status: 0 of 23 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@tpluscode

This comment has been minimized.

Copy link
Contributor

tpluscode commented Apr 17, 2018

Okay, I was thinking some more about it doesn't really matter for the client if you go backwards or forward when doing getAllMembers. After all the goal is to fetch them all eventually.

There could be a strategy to do the lookup in multiple ways, but probably it's overkill. I mean to go backward/forward interchangeably or "all previous first" or "all next first".

What the client API should have is a way to stop after a certain request count / items fetched in case of large collections. I would personally implement the getAllMembers as an external helper.

import BatchLoader from 'heracles/batch';

let batch = client.getResource("http://temp.uri/api/collection?page=876");

// will stop once 100 members are fetched or 20 requests are made,
// whichever comes first
batch = BatchLoader.getMoreMembers({
  follow: 'previousFirst',
  memberLimit: 100, 
  requestLimit: 20
});

// the all the rest, if anything, but not any previous pages
batch = BatchLoader.getMoreMembers({
  follow: 'nextOnly'
});

The BatchLoader would internally do the looping and use a more primitive API of fetching a individual previous or next pages.


Review status: 0 of 23 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

- partial collection view API is no more greedy
- revamped partial collection view API to be iterator-like for both next
and previous links
@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Apr 17, 2018

OK, I've revamped it a bit, to be an iterator-like:

const collection = client.getResource("http://temp.uri/api/collection"); // assuming the result has hydra:view
const view = collection.getView();
while (view.hasNextPage) { // there is also view.hasPreviousPage
  for (const item of (await view.getNextPage()).members) { // there is also view.getPreviousPage()
    // do something with item
  {
}

The view taken from the initial collection maintain it's state between consecutive calls to getNextPage/getPreviousPage so the links are updated internally according to obtained resources.

As for the BatchLoader concept - do you envision it as a separate extension (separate repo, npm package, etc.), or just a build-in helper class?


Review status: 0 of 26 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Apr 24, 2018

@lanthaler, @tpluscode, @elf-pavlik - any update on this?

@tpluscode

This comment has been minimized.

Copy link
Contributor

tpluscode commented Apr 25, 2018

As for the BatchLoader concept - do you envision it as a separate extension (separate repo, npm package, etc.), or just a build-in helper class?

I was just thinking about something included in the library itself


Reviewed 15 of 25 files at r1, 11 of 12 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


integration-tests/HydraClient.ApiDocumentation.spec.ts, line 1 at r2 (raw file):

import ApiDocumentation from "../src/DataModel/ApiDocumentation";

Why get rid of the type actually?


integration-tests/server/api.jsonld, line 24 at r2 (raw file):

        "view": {
            "first": "/api/people/page1",
            "next": "/api/people/page1",

It doesn't make sense to have first and next the same id


integration-tests/server/api/people.jsonld, line 13 at r2 (raw file):

    "view": {
        "first": "/api/people/page1",
        "next": "/api/people/page1",

Ditto


src/DataModel/IPartialCollectionPage.ts, line 7 at r2 (raw file):

 * @interface
 */
export interface IPartialCollectionPage {

I don't think this really represents the current design of Hydra. Getting a page of a collection actually returns the collection, but only a subset of its members

{
  "@id": "/collection/1",
  "member": [],
  "view": {
    "next": "/collection/2"
  }
}

A page doesn't exist as a concept. Do we need it in code?


src/DataModel/IPartialCollectionView.ts, line 9 at r2 (raw file):

 * @interface
 */
export interface IPartialCollectionView extends IResource {

Not adding actual getters for first/prev/next/last?


tests/DataModel/ICollection.spec.ts, line 82 at r2 (raw file):

    describe("by following next links", () => {
      beforeEach(
        run(async () => {

Unrelated tip: recent Jasmine supports async/await without additional boilerplate ;)


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Apr 25, 2018

Review status: all files reviewed at latest revision, 6 unresolved discussions.


integration-tests/HydraClient.ApiDocumentation.spec.ts, line 1 at r2 (raw file):

Previously, tpluscode (Tomasz Pluskiewicz) wrote…

Why get rid of the type actually?

It was easier. Previously, instance of this class was created at the end of processing, when whole graph was created. Now, when the competence of creating instances was moved elsewhere it was way easier to attach a missing method to a plain object than to create a class instance and fight with the readonly nature of most of the properties exposed.


integration-tests/server/api.jsonld, line 24 at r2 (raw file):

Previously, tpluscode (Tomasz Pluskiewicz) wrote…

It doesn't make sense to have first and next the same id

True


integration-tests/server/api/people.jsonld, line 13 at r2 (raw file):

Previously, tpluscode (Tomasz Pluskiewicz) wrote…

Ditto

True


src/DataModel/IPartialCollectionPage.ts, line 7 at r2 (raw file):

Previously, tpluscode (Tomasz Pluskiewicz) wrote…

I don't think this really represents the current design of Hydra. Getting a page of a collection actually returns the collection, but only a subset of its members

{
  "@id": "/collection/1",
  "member": [],
  "view": {
    "next": "/collection/2"
  }
}

A page doesn't exist as a concept. Do we need it in code?

Whatever you name you put to it - an iterator'like being is necessary. Either it is a page (feels natural), lot, subset or part - it will be still the same.
Also the API doesn't have to reflect all concepts on 1-to-1 basis. I use the hydra:view value to drive the behavior which actually doesn't expose the view capabilities as I felt it was unnecessary.


src/DataModel/IPartialCollectionView.ts, line 9 at r2 (raw file):

Previously, tpluscode (Tomasz Pluskiewicz) wrote…

Not adding actual getters for first/prev/next/last?

Exactly - I think these are unnecessary. It actually may be useful for the batch loader with rewinding capability - I'll have to collect IRI's of parts crawled so once the last page is reached and the crawling will have to start from the first page I will have a stop condition available.


tests/DataModel/ICollection.spec.ts, line 82 at r2 (raw file):

Previously, tpluscode (Tomasz Pluskiewicz) wrote…

Unrelated tip: recent Jasmine supports async/await without additional boilerplate ;)

Hmm. I've made a quick look at the doc - indeed it is possible to use asyncs. I think we'll end up with a separate PR for that.


Comments from Reviewable

- code review tweaks
- introduced PartialCollectionCrawler for iterating through partial
collection pages
- expanded IPartialCollectionView with link
@lanthaler

This comment has been minimized.

Copy link
Member

lanthaler commented Apr 29, 2018

Reviewed 15 of 25 files at r1, 11 of 12 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


integration-tests/HydraClient.EntryPoint.spec.ts, line 163 at r2 (raw file):

              const view = this.people.hypermedia.getView();
              while (view.hasNextPage) {
                for (const item of (await view.getNextPage()).members) {

The design would be more robust if getNextPage() wouldn't change view but only return the next page instead IMO.


src/DataModel/IPartialCollectionView.ts, line 9 at r2 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

Exactly - I think these are unnecessary. It actually may be useful for the batch loader with rewinding capability - I'll have to collect IRI's of parts crawled so once the last page is reached and the crawling will have to start from the first page I will have a stop condition available.

I think having getters would be beneficial.


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Apr 29, 2018

Review status: 16 of 28 files reviewed at latest revision, 7 unresolved discussions.


integration-tests/HydraClient.EntryPoint.spec.ts, line 163 at r2 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

The design would be more robust if getNextPage() wouldn't change view but only return the next page instead IMO.

I wanted it by design to look and work like an iterator, but I'll try to experiment with it a little bit.


src/DataModel/IPartialCollectionView.ts, line 9 at r2 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

I think having getters would be beneficial.

I've just pushed an update with these links added. Still, I'm not happy with it (links are now of type string), thus I'll try to make further changes to it.


Comments from Reviewable

@lanthaler

This comment has been minimized.

Copy link
Member

lanthaler commented May 5, 2018

Reviewed 13 of 13 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


integration-tests/HydraClient.EntryPoint.spec.ts, line 163 at r2 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

I wanted it by design to look and work like an iterator, but I'll try to experiment with it a little bit.

I see. Having some form of iterator or crawler as you just implemented is the way to go then IMO.


integration-tests/server/api/people.jsonld, line 11 at r3 (raw file):

        }
    ],
    "view": {

Could you please give this view an @id so that it's clear what page this it. Same for other files.


src/PartialCollectionCrawler.ts, line 26 at r3 (raw file):

export interface ICrawlingOptions {
  /**
   * Gets a direction of the crawling.

As this interface is primarily used to set these options, I would change the documentation to something along the line of "The crawling direction", "The maximum number of members to retrieve", etc.


src/PartialCollectionCrawler.ts, line 41 at r3 (raw file):

  /**
   * Gets a value indicating whether to rewind back to the beginning (or end) of the collection in case the starting
   * point was not the first (or last) possible view.

A cleaner design might be to allow direction to not only be forward and backward but also both.


src/PartialCollectionCrawler.ts, line 70 at r3 (raw file):

   * @returns {Promise<Iterable<IResource>>}
   */
  public async getMoreMembers(options?: ICrawlingOptions): Promise<Iterable<IResource>> {

I think getMembers might be a more appropriate name given that it includes the members of the view that was passed to from and that it is possible to restrict the number of members/requests via options.


src/PartialCollectionCrawler.ts, line 74 at r3 (raw file):

    const result = [];
    for (const item of this.collection.members) {
      result.push(item);

This shouldn't add more members than options.memberLimit


src/PartialCollectionCrawler.ts, line 102 at r3 (raw file):

      visitedPages.push(view.iri);
      for (const item of part) {
        result.push(item);

... again, this shouldn't add more members than the specified limit.


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented May 16, 2018

Review status: 15 of 28 files reviewed at latest revision, 10 unresolved discussions.


integration-tests/HydraClient.EntryPoint.spec.ts, line 163 at r2 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

I see. Having some form of iterator or crawler as you just implemented is the way to go then IMO.

OK. I've introduced both, hydra:view mapped to view property and getIterator method to obtain the iterator.
BTW - hydra:view is `hydra:Link meaning it can be obtained, but there are two issues here:

  1. IRI of that resource on the right hand side of hydra:view relation will be in most cases a blank node
  2. It is not the collection - dereferencing that resource is meaningles
  3. I don't expect server to provide that resource if it's not a blank node for non-RDF backends

integration-tests/server/api/people.jsonld, line 11 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Could you please give this view an @id so that it's clear what page this it. Same for other files.

And what page is it? Do you mean /api/people/page1? I'm unsure if that's the case. Resource under that IRI should be a hydra:Collection - assigning that IRI to that view would make that resource both hydra:Collection and hydra:PartialCollectionView. I know there is no formal constraint on this, but I don't think this is a behavior we want. This would lead us to old design when hydra:PartialCollectionView was a hydra:PagedCollection.
Please confirm this as I hesitate to make that kind of change.


src/PartialCollectionCrawler.ts, line 26 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

As this interface is primarily used to set these options, I would change the documentation to something along the line of "The crawling direction", "The maximum number of members to retrieve", etc.

Done.


src/PartialCollectionCrawler.ts, line 41 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

A cleaner design might be to allow direction to not only be forward and backward but also both.

I was thinking about it and I don't know how it should work. Currently, if there option rewind is given, while forwarding, it'll get back to first page and proceed unless visited (for backward direction it will got to last page repsectively). For both, does it mean after reaching last page (or first for backward), it should go back to the initial page and change direction?


src/PartialCollectionCrawler.ts, line 70 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

I think getMembers might be a more appropriate name given that it includes the members of the view that was passed to from and that it is possible to restrict the number of members/requests via options.

Done.


src/PartialCollectionCrawler.ts, line 74 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

This shouldn't add more members than options.memberLimit

Done.


src/PartialCollectionCrawler.ts, line 102 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

... again, this shouldn't add more members than the specified limit.

Done.


Comments from Reviewable

@lanthaler

This comment has been minimized.

Copy link
Member

lanthaler commented May 27, 2018

:lgtm:


Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


integration-tests/HydraClient.EntryPoint.spec.ts, line 163 at r2 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

OK. I've introduced both, hydra:view mapped to view property and getIterator method to obtain the iterator.
BTW - hydra:view is `hydra:Link meaning it can be obtained, but there are two issues here:

  1. IRI of that resource on the right hand side of hydra:view relation will be in most cases a blank node
  2. It is not the collection - dereferencing that resource is meaningles
  3. I don't expect server to provide that resource if it's not a blank node for non-RDF backends

No, view should point to a PartialCollectionView which is a concrete resource with a URL. It returns a subset of the collection.


integration-tests/server/api/people.jsonld, line 11 at r3 (raw file):
Yep, /api/people/page1.

Resource under that IRI should be a hydra:Collection

Nope. it should return this document as response, i.e., the PartialCollectionView /api/people/page1 whose representation contains a subset of the collection /api/people. The representation of a resource can contain information about other resources, that's totally OK and actually required in most cases.


src/PartialCollectionCrawler.ts, line 41 at r3 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

I was thinking about it and I don't know how it should work. Currently, if there option rewind is given, while forwarding, it'll get back to first page and proceed unless visited (for backward direction it will got to last page repsectively). For both, does it mean after reaching last page (or first for backward), it should go back to the initial page and change direction?

Fair enough. remaining / superseding (= forward), preceding (= backward) and all (= both) or something along those lines might indeed be better.


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented May 27, 2018

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


integration-tests/HydraClient.EntryPoint.spec.ts, line 163 at r2 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

No, view should point to a PartialCollectionView which is a concrete resource with a URL. It returns a subset of the collection.

Hmm. It means the example partial views are incorrect. Please take a look ta page1.jsonld and page2.jsonld.
I got confused with the partial views - some good examples of main collection and it's views could come in handy.


integration-tests/server/api/people.jsonld, line 11 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Yep, /api/people/page1.

Resource under that IRI should be a hydra:Collection

Nope. it should return this document as response, i.e., the PartialCollectionView /api/people/page1 whose representation contains a subset of the collection /api/people. The representation of a resource can contain information about other resources, that's totally OK and actually required in most cases.

As I wrote in the previous comment - please review the partial views and let's craft some good examples of main collection and its views as I got confused and I feel I don't understand the construct correctly.


src/PartialCollectionCrawler.ts, line 41 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Fair enough. remaining / superseding (= forward), preceding (= backward) and all (= both) or something along those lines might indeed be better.

I assume current design is OK?


Comments from Reviewable

@lanthaler

This comment has been minimized.

Copy link
Member

lanthaler commented Jun 3, 2018

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


integration-tests/HydraClient.ApiDocumentation.spec.ts, line 1 at r2 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

It was easier. Previously, instance of this class was created at the end of processing, when whole graph was created. Now, when the competence of creating instances was moved elsewhere it was way easier to attach a missing method to a plain object than to create a class instance and fight with the readonly nature of most of the properties exposed.

@tpluscode, could you please mark your comments as resolved if you are happy with the replies/changes. Thanks


integration-tests/HydraClient.EntryPoint.spec.ts, line 163 at r2 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

Hmm. It means the example partial views are incorrect. Please take a look ta page1.jsonld and page2.jsonld.
I got confused with the partial views - some good examples of main collection and it's views could come in handy.

Done.. sorry that I missed this before


integration-tests/server/api/people.jsonld, line 11 at r3 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

As I wrote in the previous comment - please review the partial views and let's craft some good examples of main collection and its views as I got confused and I feel I don't understand the construct correctly.

Left some comments on on page1.jsonld and page2.jsonld


integration-tests/server/api/people/page1.jsonld, line 3 at r4 (raw file):

{
    "@context": "/api/context.jsonld",
    "@id": "/api/people/page1",

This needs to be changed from /api/people/page1 to /api/people, otherwise you have N collections instead of a single paginated one.


integration-tests/server/api/people/page1.jsonld, line 11 at r4 (raw file):

        }
    ],
    "view": {

... and this view will be /api/people/page1. So it's just a resource that embeds a representation of /api/people (a subset thereof)


integration-tests/server/api/people/page2.jsonld, line 3 at r4 (raw file):

{
    "@context": "/api/context.jsonld",
    "@id": "/api/people/page2",

Same applies as for page1


src/PartialCollectionCrawler.ts, line 41 at r3 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

I assume current design is OK?

Yes, we can leave it as is I guess


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Jun 12, 2018

I've updated implementation to address latest feedback. I hope we're close to get it done and resolved.


Review status: 21 of 28 files reviewed, 8 unresolved discussions (waiting on @lanthaler and @tpluscode)


integration-tests/server/api/people.jsonld, line 11 at r3 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Left some comments on on page1.jsonld and page2.jsonld

Done.


integration-tests/server/api/people/page1.jsonld, line 3 at r4 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

This needs to be changed from /api/people/page1 to /api/people, otherwise you have N collections instead of a single paginated one.

Done.


integration-tests/server/api/people/page1.jsonld, line 11 at r4 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

... and this view will be /api/people/page1. So it's just a resource that embeds a representation of /api/people (a subset thereof)

Done.


integration-tests/server/api/people/page2.jsonld, line 3 at r4 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Same applies as for page1

Done.


Comments from Reviewable

@lanthaler

This comment has been minimized.

Copy link
Member

lanthaler commented Jun 15, 2018

This is ready to merge as soon as @tpluscode is happy with the changes and resolves his comments


Reviewed 1 of 23 files at r1, 7 of 7 files at r5.
Review status: all files reviewed, 4 unresolved discussions (waiting on @tpluscode)


Comments from Reviewable

@alien-mcl

This comment has been minimized.

Copy link
Member Author

alien-mcl commented Jul 2, 2018

Review status: all files reviewed, 4 unresolved discussions (waiting on @tpluscode)


integration-tests/HydraClient.ApiDocumentation.spec.ts, line 1 at r2 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

@tpluscode, could you please mark your comments as resolved if you are happy with the replies/changes. Thanks

Can we move on with this, @tpluscode?


Comments from Reviewable

…on IPartialCollectionIterator
Copy link
Member Author

alien-mcl left a comment

Reviewable status: 18 of 29 files reviewed, 4 unresolved discussions (waiting on @lanthaler and @tpluscode)


src/DataModel/IPartialCollectionView.ts, line 9 at r2 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

I've just pushed an update with these links added. Still, I'm not happy with it (links are now of type string), thus I'll try to make further changes to it.

I've introduces PartialCollectionView equivalent and refactored some of this iterator's property names.

Copy link
Member

lanthaler left a comment

Travis is currently failing. Could you please fix it so that we can proceed and merge this

Reviewed 11 of 11 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tpluscode and @alien-mcl)


integration-tests/HydraClient.ApiDocumentation.spec.ts, line 1 at r2 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

Can we move on with this, @tpluscode?

last call @tpluscode, otherwise I'll merge it without your feedback


src/DataModel/IPartialCollectionIterator.ts, line 12 at r6 (raw file):

   * Gets the IRI of current part.
   */
  readonly current: string;

Please rename this to currentPartIri to be consistent with all the other properties

Copy link
Member Author

alien-mcl left a comment

I cannot reproduce the issue - fresh npm install and npm run test works OK. I'm not sure what is wrong - need some time to dig as it seems runtime environment related (I'm runnit node 8, travis is on node 10)

Reviewable status: 26 of 29 files reviewed, 5 unresolved discussions (waiting on @lanthaler and @tpluscode)


src/DataModel/IPartialCollectionIterator.ts, line 12 at r6 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Please rename this to currentPartIri to be consistent with all the other properties

Done.

Copy link
Member Author

alien-mcl left a comment

I was able to reproduce travis' issue locally and it should be fixed now.

Reviewable status: 26 of 30 files reviewed, 5 unresolved discussions (waiting on @lanthaler and @tpluscode)

Copy link
Member

lanthaler left a comment

Thanks Karol

Reviewed 4 of 4 files at r7.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @tpluscode)

@lanthaler lanthaler merged commit 5ea465e into HydraCG:use-case/2_1_and_2_2_api_documentation Jul 31, 2018
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 4 discussions left (tpluscode)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (lanthaler) No manifest changes detected
lanthaler added a commit that referenced this pull request Jul 31, 2018
* Added routines gathering all members by traversing partial collection view links
* Added support for obtaining all members from a collection that is not a partial view
* Style fixes
* Fixing partial collection view implementation to the latest official approach
* Changes after code review, including:
- partial collection view API is no more greedy
- revamped partial collection view API to be iterator-like for both next and previous links
* Several changes including:
- code review tweaks
- introduced PartialCollectionCrawler for iterating through partial collection pages
- expanded IPartialCollectionView with link
* Changes after code review.
* Some few more changes after code review.
* More feedback changes
* Introduced IPartialCollectionView and refactored some IRI properties on IPartialCollectionIterator
* Changed 'current' to 'currentPartIri' property of IPartialCollectionIterator
* Fixed issue with server.ts causing transpilation to fail new node v10
lanthaler added a commit that referenced this pull request Jul 31, 2018
* Added support for API documentation data structure description
* Added indirect typing support
* Some minor code review changes.
* Changes after code review with major change related to embedding hydra ontology for indirect typing
* Improve operation invocation API with IRI templates
* Added features:
  - implementation for issue #31
  - introduced HydraClientFactory
This closes #31
* Obtaining all collection members fixing issue 33 (#34)
* Added routines gathering all members by traversing partial collection view links
* Added support for obtaining all members from a collection that is not a partial view
* Style fixes
* Fixing partial collection view implementation to the latest official approach
* Changes after code review, including:
- partial collection view API is no more greedy
- revamped partial collection view API to be iterator-like for both next and previous links
* Several changes including:
- code review tweaks
- introduced PartialCollectionCrawler for iterating through partial collection pages
- expanded IPartialCollectionView with link
* Changes after code review.
* Some few more changes after code review.
* More feedback changes
* Introduced IPartialCollectionView and refactored some IRI properties on IPartialCollectionIterator
* Changed 'current' to 'currentPartIri' property of IPartialCollectionIterator
* Fixed issue with server.ts causing transpilation to fail new node v10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.