Skip to content

Added default_skip_when_empty option as config using the exclusionStrategy - #1257

Open
vasilake-v wants to merge 4 commits into
schmittjoh:masterfrom
vasilake-v:add_skip_when_empty_default_option
Open

Added default_skip_when_empty option as config using the exclusionStrategy#1257
vasilake-v wants to merge 4 commits into
schmittjoh:masterfrom
vasilake-v:add_skip_when_empty_default_option

Conversation

@vasilake-v

Copy link
Copy Markdown

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

goetas commented Nov 8, 2020

Copy link
Copy Markdown
Collaborator

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?

Comment thread src/Context.php
Comment thread src/Context.php
Comment thread src/Exclusion/ExpressionLanguageExclusionStrategy.php
Comment thread src/Metadata/Driver/AnnotationDriver.php Outdated
Comment thread tests/Handler/ArrayCollectionHandlerTest.php
@vasilake-v

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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