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

UX: Provide full diff for code samples #3825

Merged
merged 1 commit into from
Jun 10, 2018

Conversation

keradus
Copy link
Member

@keradus keradus commented Jun 9, 2018

Our code samples are super condensed and each line of them has a purpose.

For example, diff for first code sample of native_function_invocation is:

 * Example #1. Fixing with the default configuration.
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -2,9 +2,9 @@
    
    function baz($options)
    {
   -    if (!array_key_exists("foo", $options)) {
   +    if (!\array_key_exists("foo", $options)) {
            throw new \InvalidArgumentException();
        }
    
   -    return json_encode($options);
   +    return \json_encode($options);
    }
   
   ----------- end diff -----------

The second sample is this:

 * Example #2. Fixing with configuration: ['exclude' => ['json_encode']].
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -2,7 +2,7 @@
    
    function baz($options)
    {
   -    if (!array_key_exists("foo", $options)) {
   +    if (!\array_key_exists("foo", $options)) {
            throw new \InvalidArgumentException();
        }
    
   
   ----------- end diff -----------

in fact, they are using very same input code, just different configuration. But in later, we don't know that, as we don't see json_encode in diff at all.


Proposed PR would change 2nd example into:

 * Example #2. Fixing with configuration: ['exclude' => ['json_encode']].
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -1,10 +1,10 @@
    <?php
    
    function baz($options)
    {
   -    if (!array_key_exists("foo", $options)) {
   +    if (!\array_key_exists("foo", $options)) {
            throw new \InvalidArgumentException();
        }
    
        return json_encode($options);
    }
   
   ----------- end diff -----------

@keradus
Copy link
Member Author

keradus commented Jun 9, 2018

@SpacePossum , you made whole differ, what do you think ?

@SpacePossum
Copy link
Contributor

SpacePossum commented Jun 9, 2018

👍 for the idea, we could even create a real full differ on sebastianbergmann/diff one day (I can write one in PHP-CS-Fixer/diff, should be easy).

@keradus
Copy link
Member Author

keradus commented Jun 10, 2018

What would be the difference in generated output between differ in this PR and that full differ you imagine about? TBH, not sure if there is a need - for udiffer, it was important for us, we wanted to use it heavily and in the and, a lot of ppl were looking for udiffer implementation in PHP.
But full differ ? Well, there is not that much usage of it in the wild, and here, at our code samples case, it's super edge case.
Long story short - not sure if it's worth to spend time on it

@keradus keradus added this to the 2.12.1 milestone Jun 10, 2018
@keradus keradus merged commit db83eae into PHP-CS-Fixer:2.12 Jun 10, 2018
keradus added a commit that referenced this pull request Jun 10, 2018
This PR was squashed before being merged into the 2.12 branch (closes #3825).

Discussion
----------

UX: Provide full diff for code samples

Our code samples are super condensed and each line of them has a purpose.

For example, diff for first code sample of `native_function_invocation` is:
```
 * Example #1. Fixing with the default configuration.
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -2,9 +2,9 @@

    function baz($options)
    {
   -    if (!array_key_exists("foo", $options)) {
   +    if (!\array_key_exists("foo", $options)) {
            throw new \InvalidArgumentException();
        }

   -    return json_encode($options);
   +    return \json_encode($options);
    }

   ----------- end diff -----------
```

The second sample is this:
```
 * Example #2. Fixing with configuration: ['exclude' => ['json_encode']].
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -2,7 +2,7 @@

    function baz($options)
    {
   -    if (!array_key_exists("foo", $options)) {
   +    if (!\array_key_exists("foo", $options)) {
            throw new \InvalidArgumentException();
        }

   ----------- end diff -----------
```

in fact, they are using very same input code, just different configuration. But in later, we don't know that, as we don't see `json_encode` in diff at all.

------------------------

Proposed PR would change 2nd example into:
```
 * Example #2. Fixing with configuration: ['exclude' => ['json_encode']].
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -1,10 +1,10 @@
    <?php

    function baz($options)
    {
   -    if (!array_key_exists("foo", $options)) {
   +    if (!\array_key_exists("foo", $options)) {
            throw new \InvalidArgumentException();
        }

        return json_encode($options);
    }

   ----------- end diff -----------
```

Commits
-------

db83eae UX: Provide full diff for code samples
@keradus keradus deleted the 2.12_fulldiff branch June 10, 2018 08:23
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