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

add identifiers to graph elements #124

Merged
merged 6 commits into from
Jun 3, 2020
Merged

add identifiers to graph elements #124

merged 6 commits into from
Jun 3, 2020

Conversation

Gummibeer
Copy link
Collaborator

@Gummibeer Gummibeer commented Nov 20, 2019

fixes #73

In addition to the multiple instances itself this comes with two more major changes.

  • the Graph does not extend the BaseType anymore - this is because they got too different and doesn't overlap enough anymore.
  • the hide() and show() logic on the Graph got much more complex with the goal to let it do what the user (or I) would expect it to do if multiple calls with different parameters are chained.
    • ->hide(Org::class, null)->hide(Org::class, 'spatie') won't only hide spatie but simply ignore the second call because the first one already ignores all orgs.
    • ->hide(Org::class, null)->show(Org::class, 'spatie') is the most complex one - it will keep all existing orgs hidden except the specific spatie one.
      At all the hide() and show() methods should be called after adding all instances to make this work like expected.
$graph = new Graph();
$graph->organization('example1');
$graph->organization('spatie');
$graph->hide(Organization::class, null);
$graph->show(Organization::class, 'spatie'):
$graph->organization('example2');

Would result in two shown organizations (spatie & example2) because the last one is added after the hide all configuration is overridden by the specific hide/show configuration.

But without all this we would receive even more unexpected results. Right now there is no weighting - so simply the last call will set the state. So you should be careful in which order you hide/show your graph elements.

@Gummibeer Gummibeer self-assigned this Nov 20, 2019
@Gummibeer
Copy link
Collaborator Author

@sebastiandedeyne sorry, have missed the approval.
Should we release it as minor or major? The graph base class has changed. So possible typehints will fail.

And because the graph gets more and more complex should we add this package to the official spatie docs instead of a readme only?

@desaintflorent
Copy link

Hello, I currently have a similar need where I would need multiple instance of the same type in a graph. we should be able to have different instance of a type on a graph. Do you now when this PR will be merged or how I can achieve that in the meantime ?

@Gummibeer
Copy link
Collaborator Author

Gummibeer commented Apr 14, 2020

@desaintflorent until now I never needed the same type multiple times on top-level!? 🤔
This PR is in the main repo so you can check it out via dev-ft-graph-identifier as package version.
If this solves your issue or you find any bugs or something feel free to send in feedback. Everyone who can confirm that it works or not is helpful. In fact I have the "power" to merge and release it. But I won't do it until an approve by @sebastiandedeyne or some users of the package.
If you only need the types on a lower level, for example as author of a creative work you can use the Schema::person() directly.

EDIT: okay, it's approved, I only wait for feedback.
@desaintflorent do you have an opinion regarding the release (minor or major)?
The docs decision isn't blocking and would be done in a different PR anyway.

@desaintflorent
Copy link

@Gummibeer My need is to have two ItemList on the top level. Minor sounds enough I guess. Thank you for taking care of this 👍 I will keep you posted if It solve my problem without any issue.

@logan-jobzmall
Copy link

logan-jobzmall commented Jun 2, 2020

Looking forward to this. Any news on the release? I will also jump in check to see if this helps my case as well. The example of https://schema.org/Collection shows multiple books with different IDs - this is super necessary.

@logan-jobzmall
Copy link

News on my end @Gummibeer: Works very well. I was able to generate a list of VideoObjects and add them to the graph. As stated earlier, my use case needs the ability to create a collection object and add that to it as well - but I was able to construct that object myself for now (that is a different ticket to add Collection as a type)

@Gummibeer
Copy link
Collaborator Author

@logan-jobzmall happy to hear, I will release it today then.
Regarding your collection issue - the Collection is part of the bib extension.

Defined in the bib section.

So the matching issue is #25 and PR #80 .
Have to check again but I believe that the open question was if we generate all extensions by default or provide a console command to allow the user to do so, if so where/how to store the files and so on.
Feel free to hook into this discussion and provide some insights in your usecase, expectations and so on. It always helps as a maintainer to get some insights in the real world usage of this package.

@Gummibeer Gummibeer merged commit 3c06185 into master Jun 3, 2020
@Gummibeer Gummibeer deleted the ft-graph-identifier branch June 3, 2020 12:54
@Gummibeer
Copy link
Collaborator Author

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.

Graph with multiple instances of same type
4 participants