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

JsonLdMulti - Remove requirement for 2 items #276

Merged
merged 2 commits into from
Jul 19, 2022
Merged

Conversation

J-Brk
Copy link
Collaborator

@J-Brk J-Brk commented Apr 25, 2022

JsonLdMulti does not output anything on default; this is inconsistent with the use of JsonLd.
The impact is high because this makes JsonLdMulti not be usable as default replacement for JsonLd.

This pull request removes the check for minimum 2 items.

Q A
Is bugfix? ✔️
New feature?
Breaks BC?
Tests pass? ✔️
Fixed issues #275

@J-Brk J-Brk marked this pull request as ready for review April 25, 2022 11:42
@J-Brk J-Brk added the bug label Apr 25, 2022
@J-Brk J-Brk changed the title Bug - JsonLdMulti - Remove requirement for 2 items JsonLdMulti - Remove requirement for 2 items Apr 25, 2022
@vinicius73
Copy link
Member

LGTM;
But tests will be welcome to prevent future regressions.

Implementing unit test for single record output
@J-Brk
Copy link
Collaborator Author

J-Brk commented Apr 27, 2022

Is it correct that Travis CI is not running? I don't know if Scrutinizer covers running all the tests.

@vinicius73
Copy link
Member

Is it correct that Travis CI is not running? I don't know if Scrutinizer covers running all the tests.

We must configure github actions :(

@bradyrenting
Copy link

bradyrenting commented Jul 9, 2022

Hey guys, I ran into the same issue as #275. What's the status of this PR?

Thanks! 😄

@axyr
Copy link

axyr commented Aug 23, 2022

This broke my existing implementation 😅

@vinicius73 vinicius73 deleted the JsonLdMulti-patch-1 branch August 24, 2022 00:09
@J-Brk
Copy link
Collaborator Author

J-Brk commented Aug 27, 2022

@axyr I'm sorry to hear that. This fix make it behave as it it should be and as it is described in the documentation.
Can you tell me how it broke your implementation?

@axyr
Copy link

axyr commented Aug 28, 2022

Hi, No problem at all, just wanted to let you know!

The tags stopped being printed in the blade template.

I haven't got to the bottom to it yet, as downgrading was the fasted solution.

Thanks for this nice package!

@J-Brk
Copy link
Collaborator Author

J-Brk commented Aug 29, 2022

@axyr Maybe your issue was related to bug: #283 ? It has been resolved in the new version 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants