Skip to content

Use constructor property promotion in monolog_console.rst code example #18039

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

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

timkrase
Copy link
Contributor

As described in the PR title

Comment on lines +48 to 49
public function __construct(private LoggerInterface $logger)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function __construct(private LoggerInterface $logger)
{
public function __construct(
private LoggerInterface $logger
){

Copy link
Contributor Author

@timkrase timkrase Mar 10, 2023

Choose a reason for hiding this comment

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

So

public function __construct(
   private Service $service
) {
}

is the formatting that should be used in the doc code examples?

Just asking because before opening the PR I found this disscusion in PR #17204 where you concluded that cpp should be done in the same code style as in the symfony code. And both the examples linked in the PR as well as previous PRs to the docs are formatted like:

public function __construct(private Service $service)
{
}

I'm just curious what's the preferred style for the docs going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, I think we keep them in one line if we have only on, but sometimes we keep many on one line too.

@javiereguiluz not sure how we should proceed in general with property promotion? As Code is meant to be executed and docs should be readable, I think we should line break them all the time.

WDYT @symfony/team-symfony-docs ?

Copy link
Member

Choose a reason for hiding this comment

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

I’d vote for a pragmatic middle based on line length: of the code gets too long (around the 70th char?) we break all args onto multiple lines. That’s usually what I do and it’ll keep the code blocks without needing to scroll horizontally

Copy link
Member

@wouterj wouterj Mar 10, 2023

Choose a reason for hiding this comment

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

We've rejected CPP before: #17204 (comment) & #16087 (comment) But we also merged PRs adding CP... so I'm not sure what our policy is now.

About code style: we always follow Symfony CS (except that we care a bit more about not too long lines). In the Symfony CS, property promotion must always be on a multi line argument list.

Copy link
Contributor

Choose a reason for hiding this comment

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

CCP makes Code examples shorter and less noisy, so we should go for it.

👍 for multiline for readability

Copy link
Contributor

@OskarStark OskarStark Mar 11, 2023

Choose a reason for hiding this comment

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

In this PR @nicolas-grekas is going the same direction

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed it while merging, thanks

@OskarStark
Copy link
Contributor

Thank you @timkrase.

@OskarStark OskarStark merged commit e890d84 into symfony:6.2 Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants