Skip to content

[Serializer] Add missing options context #14384

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 11, 2021
Merged

[Serializer] Add missing options context #14384

merged 1 commit into from
Jan 11, 2021

Conversation

maxhelias
Copy link
Contributor

@maxhelias maxhelias commented Oct 9, 2020 •
edited
Loading

The options are not really in the right place since just above the text indicates the elements for associative arrays.
But we only list context options for an associative array here : https://symfony.com/doc/current/components/serializer.html#context

Wouldn't we move all the options for all encoders here ? https://symfony.com/doc/current/components/serializer.html#encoders
And in the first section display only the possible options for associative array with the link to their description.

Solve #10277 and #10286

@maxhelias maxhelias marked this pull request as draft October 9, 2020 15:25
@maxhelias
Copy link
Contributor Author

This PR can be transformed to document the context options of each serialization format #8505 but i need feedback 😄

@javiereguiluz
Copy link
Member

@maxhelias minor comment: there's no need to create "draft PRs" because in Symfony we consider all PRs already "in draft" and "ready to review" until they are merged. In the code repository we do the same and we don't use draft PRs. Thanks!

@maxhelias
Copy link
Contributor Author

@javiereguiluz ok I'll change then.
I spotted that it can be linked to this issue #11786 and PR #13269

@maxhelias maxhelias marked this pull request as ready for review October 9, 2020 15:49
@maxhelias
Copy link
Contributor Author

Status: Needs Work

@wouterj
Copy link
Member

wouterj commented Nov 5, 2020

Fyi, I've just merged the PR you mentioned. However, I've also opened another PR that changes this: #14521 After that, this PR should probably be rebased.

@maxhelias
Copy link
Contributor Author

@wouterj I rebase the branch and I harmonize a little more the Normalizers & Encoders section.
Maybe it doesn't belong in this PR, tell me and I'll open another one if needed.

@wouterj
Copy link
Member

wouterj commented Nov 12, 2020

About the DOCtor errors your seeing: I reported this a couple days: OskarStark/doctor-rst#808 . Normally, we add 2 spaces between the columns in these tables, but to avoid this error I changed it to 1 space for one column :)

@maxhelias maxhelias force-pushed the options-context branch 2 times, most recently from 0850560 to d7a7cc2 Compare November 12, 2020 15:14
@maxhelias
Copy link
Contributor Author

Thanks for the tips, it's fixed 👍

@maxhelias
Copy link
Contributor Author

friendly ping @wouterj 😃

wouterj
wouterj approved these changes Nov 30, 2020
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, somehow this PR was no longer in my notification center.

I've one minor syntax fix, which we can also fix while merging. Are all these options introduced before Symfony 4.4, or do we need a .. versionadded:: 4.4 directive for new options?

@maxhelias
Copy link
Contributor Author

Don't worry about it. The most recent one has been integrated in version 4.1, do you want the directive anyway ?

@OskarStark
Copy link
Contributor

The correct versionadded directives would be super helpful if you can find them out 🙏

@maxhelias
Copy link
Contributor Author

maxhelias commented Dec 13, 2020 •
edited
Loading

@wouterj @OskarStark I added the right directive

OskarStark
OskarStark approved these changes Dec 14, 2020
View reviewed changes
@maxhelias
Copy link
Contributor Author

friendly ping @wouterj or @OskarStark 😃

wouterj added a commit that referenced this pull request Jan 11, 2021
@wouterj
[#14384] Minor grammar fix in versionadded directive
f97888b
@wouterj wouterj merged commit f1372f7 into symfony:4.4 Jan 11, 2021
wouterj added a commit that referenced this pull request Jan 11, 2021
@wouterj
Merge branch '4.4' into 5.1
b7f6cce 
* 4.4:
  [#14384] Minor grammar fix in versionadded directive
  Add missing options context
wouterj added a commit that referenced this pull request Jan 11, 2021
@wouterj
Merge branch '5.1' into 5.2
b72c7d4 
* 5.1:
  Removed 4.2 versionadded directive
  [#14384] Minor grammar fix in versionadded directive
  Add missing options context
wouterj added a commit that referenced this pull request Jan 11, 2021
@wouterj
Merge branch '5.2' into 5.x
0945da0 
* 5.2:
  Removed 4.2 versionadded directive
  [#14384] Minor grammar fix in versionadded directive
  Add missing options context
@wouterj
Copy link
Member

wouterj commented Jan 11, 2021

Thank you @maxhelias (also thank you for your patience!)! Lots of great new content in this PR :)

This was referenced Jan 11, 2021
Closed
Closed
@maxhelias maxhelias mentioned this pull request Jan 11, 2021
Closed
This was referenced Oct 27, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterj wouterj wouterj approved these changes

@OskarStark OskarStark OskarStark approved these changes

Assignees
No one assigned
Labels
Serializer Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maxhelias @javiereguiluz @wouterj @OskarStark @carsonbot