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

Added default_skip_when_empty option as config using the exclusionStrategy #1257

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vasilake-v
Copy link

Extended the context to include the option to enable SkipWhenEmpty as a default behaviour for properties.

The ExclusionStrategy approach was used to enable this.

It is also related to schmittjoh/JMSSerializerBundle#789 And I will prepare a pull request for that repo. also when this will be merged

Q A
Bug fix? no
New feature? yes
Doc updated yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets schmittjoh/JMSSerializerBundle#789
License MIT

@vasilake-v vasilake-v force-pushed the add_skip_when_empty_default_option branch from e0adc9d to 25ca44d Compare October 25, 2020 23:11
@vasilake-v vasilake-v marked this pull request as ready for review October 26, 2020 07:56
@goetas
Copy link
Collaborator

goetas commented Nov 8, 2020

Hi, thanks for the PR. I like the idea, but I'm not sure that works as expected. IMO it will skip all the props when enabled. To check it, can you please also add few props in ObjectWithEmptyArrayAndHashNotAnnotated that are not empty, and check that those do not get skipped?

@vasilake-v
Copy link
Author

@goetas , you were right. After adding the test tests/Fixtures/ObjectWithEmptyArrayAndHashNotAnnotated.php i realized where was the mistake.
I did some refactorings of the implementation and, basically, extended the Exclusion mechanism with additional interface that describes signature for an exclusion method that would accept a property value also, whenever the case.

i came to this implementation, which seems cleaner in my view. I considering the:

  • approach of expressionExclusionStrategy private property assigned in constructor -- bad: one more prop. polouting class
  • extending the original ExclusionStrategyInterface with another method -- bad: other classes would have to implement the method

Maybe there are any other ideas that i haven't thought about ?

@vasilake-v
Copy link
Author

Hi, thanks for the PR. I like the idea, but I'm not sure that works as expected. IMO it will skip all the props when enabled. To check it, can you please also add few props in ObjectWithEmptyArrayAndHashNotAnnotated that are not empty, and check that those do not get skipped?

Hi @goetas Do you think we could go forward with this pull request, or are there any other concerns that i could fix ?

Thanks!

slava-v added 4 commits May 24, 2022 09:01
Adjusted the tests for the expected behaviour
…also

* Adapted the getExclusionStrategy (in the Context.php) to return DisjunctionExclusionStratgy interface so it could also accept calls to methods with other interfaces beside the ExclusionStrategyInterface
* Null value could be eventually adapted also with this kind of Exclusion strategy
@vasilake-v vasilake-v force-pushed the add_skip_when_empty_default_option branch from a450158 to 4c230d2 Compare May 24, 2022 07:03
@vasilake-v
Copy link
Author

Hi @goetas i've rebased the pull-request branch.

I'll check if i can improve/lower the complexity reported for
SerializationGraphNavigator

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.

2 participants