Skip to content

chore: move rector from require-dev to require#114

Merged
kenjis merged 2 commits into
codeigniter4:developfrom
kenjis:move-rector-to-require
Sep 20, 2023
Merged

chore: move rector from require-dev to require#114
kenjis merged 2 commits into
codeigniter4:developfrom
kenjis:move-rector-to-require

Conversation

@kenjis

@kenjis kenjis commented Sep 11, 2023

Copy link
Copy Markdown
Member

Description
See #111 (comment)

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • [] User guide updated
  • [] Conforms to style guide

@kenjis kenjis mentioned this pull request Sep 11, 2023

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A consideration: since DevKit is available for anyone to use, do we want to be locking the Rector version? If we do, we end up managing this repo and could bottleneck other people's upgrades. If we don't, we would need to lock it in our own repos to whatever config we are using.

@MGatner

MGatner commented Sep 12, 2023

Copy link
Copy Markdown
Member

@samsonasik opinion as well

@samsonasik samsonasik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good

@kenjis

kenjis commented Sep 13, 2023

Copy link
Copy Markdown
Member Author

@MGatner @samsonasik Ah, we cannot lock the version in this repo?
If we do, other people who use this cannot upgrade their rector version.

If we don't, we would need to lock it in our own repos to whatever config we are using.

Yes.

@kenjis

kenjis commented Sep 13, 2023

Copy link
Copy Markdown
Member Author

After all, since rector is not yet stable, the best practice is for each project to lock in a version. Otherwise, rector.php may stop working.

If we lock the version in this repository, and even if we constantly update to the latest version, rector.php in the user's repository will not be automatically updated, which could cause rector to stop working.

I'm starting to feel like we should leave require-dev as it is.

@samsonasik

Copy link
Copy Markdown
Member

@samsonasik

Copy link
Copy Markdown
Member

When new rector released, pin version need to be updated (or not if cause error),

If pin updated, then create new release of dev kit

@datamweb

datamweb commented Sep 13, 2023

Copy link
Copy Markdown
Contributor

If pin updated, then create new release of dev kit

last release of dev Dec 22, 2022 but rector new version every week:smiley:. dev cannot act like reactor.

@MGatner

MGatner commented Sep 13, 2023

Copy link
Copy Markdown
Member

Right. I don't want us to be managing releases here just around Rector. My opinion: we include Rector with ^ and let devs handle their own version locking (or not). Most of my smaller repo projects run fine between Rector patches and aren't worth locking to a specific version - I just go fix them when Dependabot breaks a pipeline (e.g. deprecated rule).

@kenjis kenjis force-pushed the move-rector-to-require branch from 4fedeb5 to e089451 Compare September 14, 2023 00:17
@kenjis

kenjis commented Sep 14, 2023

Copy link
Copy Markdown
Member Author

@MGatner I changed to ^0.18.3.

@kenjis kenjis force-pushed the move-rector-to-require branch from 003439f to 4b4e0d6 Compare September 14, 2023 00:22
@kenjis kenjis requested a review from MGatner September 14, 2023 00:25
@kenjis

kenjis commented Sep 14, 2023

Copy link
Copy Markdown
Member Author

I think we can release a new devkit after merging this PR.

@kenjis kenjis merged commit 92f977d into codeigniter4:develop Sep 20, 2023
@kenjis kenjis deleted the move-rector-to-require branch September 20, 2023 01:22
@MGatner MGatner mentioned this pull request Feb 22, 2024
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