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

Code style - chain calls #217

Closed
arogachev opened this issue Jul 26, 2022 · 15 comments
Closed

Code style - chain calls #217

arogachev opened this issue Jul 26, 2022 · 15 comments
Labels

Comments

@arogachev
Copy link
Contributor

arogachev commented Jul 26, 2022

Similar to #yiisoft/html#132.

$this->assertFalse($validator
->validate($formModel)
->isValid());

Possible fix:

$isValid = $validator
    ->validate($formModel)
    ->isValid();
$this->assertFalse($isValid);
@arogachev arogachev added the type:task Task label Jul 26, 2022
@vjik
Copy link
Member

vjik commented Jul 26, 2022

Or so:

$this->assertFalse(
    $validator 
        ->validate($formModel) 
        ->isValid()
); 

@arogachev
Copy link
Contributor Author

@sankaest Do you want to do it and have time for this?

@sankaest
Copy link
Contributor

no problem, I'll do (will check all packages again) it by the end of this week :)

@arogachev
Copy link
Contributor Author

arogachev commented Aug 10, 2022

Great. 👍 But @rustamwin and @darkdef have different opinion. 🤔 Maybe wait for them to share it to avoid extra work.

@sankaest
Copy link
Contributor

Any clues for me regarding style? What exactly should be changed and examples (from->to). @arogachev @rustamwin @darkdef

@darkdef
Copy link

darkdef commented Aug 15, 2022

I think that reading one line of 80 characters is easier than winding two screens.
Nobody writes like this:

function tttt(
string $str
) {

@sankaest
Copy link
Contributor

@darkdef could you please copy here "bad" code style (real example, or give row number in the class) and how it should be restyled. It is always better to show how it should be, otherwise guessing a numbers with lottery will be easier, than writing a code what is good enough. :)

And is your example regarding chaining?

Also, @samdark should it be advised in some docs and/or accepted by the community?

I am confused right now, what is "correct" style.

@darkdef
Copy link

darkdef commented Aug 15, 2022

Fo example:

example1

$this->getQuoter()->quoteTableName($table)

Vs example2

$this
   ->getQuoter()
   ->quoteTableName($table)

For me - example1 better.
And readable for human.
Example2 by codestyle, but better for pc (it's my opinion)

@samdark
Copy link
Member

samdark commented Aug 15, 2022

Yes, it should be documented. Main rules are:

  1. If it is a long chain that doesn't fit line length (120 characters) then each call should on a new line.
  2. It is is a short chain, it is alright to leave it as is.

@sankaest
Copy link
Contributor

that 120 characters is something new for me (is it final decision?) . In which yiisoft/docs/blob/master/*.md file is (or should this be) mentioned? At least I could not discover it. Need to create new one?

It seems to me, that there is quite a lot of different situations, so examples in *.md should help and clarify.

@samdark
Copy link
Member

samdark commented Aug 15, 2022

https://github.com/yiisoft/docs/blob/master/010-code-style.md is a good place to mention it. It wasn't written anywhere else yet.

@samdark
Copy link
Member

samdark commented Aug 15, 2022

Done: https://github.com/yiisoft/docs/blob/master/010-code-style.md#chain-calls

@sankaest
Copy link
Contributor

sankaest commented Aug 18, 2022

@samdark what about such kind of chain? just and example (total length > 120 char).

$this->chain->withChain()->veryLongChain(['key1' => self::CONST_1, 'key2' => self::VALUE_2, 'key3' => self::CONST_VALUE_3]);

I mean we count length from $this-> till ; with everything inside brackets of chains?
Without inside brackets it would be less, than 120 chars.

$this
    ->chain
    ->withChain()
    ->veryLongChain(['key1' => self::CONST_1, 'key2' => self::VALUE_2, 'key3' => self::CONST_VALUE_3]);

Just to be sure.

@vjik
Copy link
Member

vjik commented Aug 18, 2022

String length included all symbols (including prefix spaces).

@vjik
Copy link
Member

vjik commented Dec 1, 2023

Already fixed.

@vjik vjik closed this as completed Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants