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

apply rector: set variable name to camel case #3680

Merged
merged 18 commits into from
Sep 29, 2020

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Sep 25, 2020

@MGatner here I try apply variable name to camel case checker with Rector.

Checklist:

  • Securely signed commits
  • Conforms to style guide

@michalsn
Copy link
Member

From the first look - I think we would need a rule that leaves the initial underscore, e.g. replacing $_str variable to $str is not desired.

@samsonasik
Copy link
Member Author

we can extends the rule with skip variable with _ in first char if needed.

@samsonasik
Copy link
Member Author

@michalsn I've added utils/Rector/UnderscoreToCamelCaseLocalVariableNameRector.php for skip '_' in first character.

@MGatner
Copy link
Member

MGatner commented Sep 25, 2020

This is looking great! I'm going to apply this to a few projects as well.

@michalsn
Copy link
Member

This looks really good. Only Travis seem to have some hiccup - all tests hanged at the same time. We have to try to rerun this.

@samsonasik
Copy link
Member Author

close reopen to restart travis build

@samsonasik
Copy link
Member Author

it seems the dependency cause a very long running phpunit

@samsonasik
Copy link
Member Author

trying with distributed rector-prefixed phar with require-dev nette/utils

@samsonasik
Copy link
Member Author

Travis build green 🎉

@michalsn
Copy link
Member

Yay, you made the tests work!

I think this is a good start and this can be merged.

It would be great if we could check the possibility of handling the parameters for functions as well in the separate PR. @MGatner do you think that this type of change would be a good idea?

Of course there is no hurry.

@samsonasik
Copy link
Member Author

@michalsn I've updated to update both parameter and @param docblock

@samsonasik samsonasik changed the title apply rector: set local variable name to camel case apply rector: set variable name to camel case Sep 28, 2020
@samsonasik
Copy link
Member Author

All green 🎉 🎉 🎉

@michalsn
Copy link
Member

It looks great to me! Let's wait if others have anything to add.


- name: Validate composer.json
run: composer validate --strict

Copy link
Member

Choose a reason for hiding this comment

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

This should use the Composer cache so it can benefit from all the other running workflows. E.g from test-phpstan.yml:

      - name: Get composer cache directory
        id: composer-cache
        run: echo "::set-output name=dir::$(composer config cache-files-dir)"

      - name: Create composer cache directory
        run: mkdir -p ${{ steps.composer-cache.outputs.dir }}

      - name: Cache composer dependencies
        uses: actions/cache@v2
        with:
          path: ${{ steps.composer-cache.outputs.dir }}
          key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.json') }}
          restore-keys: ${{ runner.os }}-composer-

Copy link
Member Author

Choose a reason for hiding this comment

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

implemented.

@samsonasik
Copy link
Member Author

I think it is ready. We can try use rector/rector instead of current alternative rector/rector-prefixed for more stable dependencies check later after #3689 as if using rector/rector, travis build is too slow.

@samsonasik samsonasik requested a review from MGatner September 29, 2020 15:01
@MGatner MGatner merged commit 5425519 into codeigniter4:develop Sep 29, 2020
@samsonasik samsonasik deleted the apply-rector-1 branch September 29, 2020 15:03
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.

4 participants