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

Implement "sync-recipes --force" option #439

Merged
merged 1 commit into from
Dec 11, 2018
Merged

Implement "sync-recipes --force" option #439

merged 1 commit into from
Dec 11, 2018

Conversation

Pierstoval
Copy link
Contributor

Closes #179

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

There should be a way to ignore the lock file also.
Either we use a single --force parameter that does both (ignore symfony.lock and overwrite existing files)
Or we create two separate arguments if worth it (add --ignore-lock).

src/Configurator/CopyFromRecipeConfigurator.php Outdated Show resolved Hide resolved
src/Configurator/EnvConfigurator.php Outdated Show resolved Hide resolved
@Pierstoval
Copy link
Contributor Author

Either we use a single --force parameter that does both (ignore symfony.lock an overwrite existing files)
Or we create two separate arguments if worth it.

What about [-f] --force and [-l] --ignore-lock?

@Pierstoval Pierstoval changed the title Implement "fix-recipes --overwrite" option Implement "fix-recipes --force" option Nov 20, 2018
src/Event/UpdateEvent.php Outdated Show resolved Hide resolved
tests/FlexTest.php Outdated Show resolved Hide resolved
tests/FlexTest.php Outdated Show resolved Hide resolved
@Pierstoval
Copy link
Contributor Author

Everything should be fixed now 👍

@Pierstoval
Copy link
Contributor Author

Pierstoval commented Nov 22, 2018

@nicolas-grekas Tests are fixed, but I have an issue with Windows, I'm sure it's mostly because of the tests and not Flex itself, but it seems that git behaves differently on Linux and Windows in certain cases. This might introduce edge cases for Windows users... but I'm not even sure that this would happen.

For example, here's a difference in terms of exit code:

On windows:

> php -r "exec('git branch', $o, $c); var_dump($o, $c);"
fatal: not a git repository (or any of the parent directories): .git
Command line code:1:
array(0) {
}
Command line code:1:
int(0)

On linux:

$ php -r "exec('git branch', \$o, \$c); var_dump(\$o, \$c);"
fatal: Not a git repository (or any of the parent directories): .git
array(0) {
}
int(128)

In the tests, this is caused by the fact that the test files are located in /tmp. I'm thinking about creating a build/ directory covered by .gitignore to put the files there (as if they're ignored we can expect the configurator to ask the user whether to overwrite the file or not), but I'm not sure.

Should I change the tests so they use files directly in the project so it's covered by Git? Or is there anything to do that would make the tests compatible with both Linux and Windows? Can we put the Flex repo on AppVeyor? 😄

src/Flex.php Outdated Show resolved Hide resolved
src/Flex.php Outdated Show resolved Hide resolved
@nicolas-grekas
Copy link
Member

Except for some minor changes, I played with the feature and this looks great to me!

The experience could be improved, eg:

  • files that are in the stash could be overridden without confirmation
  • there could be more options than just y/N when asking confirmation, eg show the diff, or edit the patch before applying, or approve all or just quit or ? to display help (very much like what git add -p does).

BUT this would be for future PRs.

👍

@Pierstoval
Copy link
Contributor Author

Pierstoval commented Nov 24, 2018

I fixed the reviews! For the next features, I can create other PRs targeted on this one so we could work on this in parallel without the need for tons of new releases, if you like 🙂

tests/FlexTest.php Outdated Show resolved Hide resolved
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nice experience - simple and pragmatic.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Validated locally work like a charm after some changed I just push-forced.
👍 Ready to go on my side!

@nicolas-grekas
Copy link
Member

2nd commit fixes symfony/symfony#29510
(fabbot failure is a false-positive)

composer.json Outdated Show resolved Hide resolved
fabpot added a commit that referenced this pull request Dec 11, 2018
This PR was merged into the 1.1-dev branch.

Discussion
----------

Rename fix-recipes to sync-recipes

Would make it a bit more obvious what the command is for IMHO.
Related to #439

Commits
-------

bd94cfe Rename fix-recipes to sync-recipes
@fabpot
Copy link
Member

fabpot commented Dec 11, 2018

Needs a rebase now that the commands has been renamed.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(rebased + improved a bit meanwhile)

@nicolas-grekas nicolas-grekas changed the title Implement "fix-recipes --force" option Implement "sync-recipes --force" option Dec 11, 2018
@nicolas-grekas
Copy link
Member

Thank you @Pierstoval.

@nicolas-grekas nicolas-grekas merged commit 939de95 into symfony:master Dec 11, 2018
nicolas-grekas added a commit that referenced this pull request Dec 11, 2018
This PR was merged into the 1.1-dev branch.

Discussion
----------

Implement "sync-recipes --force" option

Closes #179

Commits
-------

939de95 Implement "sync-recipes --force" option
@Pierstoval Pierstoval deleted the fix-recipes-overwrite branch December 11, 2018 20:29
tgalopin pushed a commit to tgalopin/flex that referenced this pull request Dec 3, 2020
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.

None yet

5 participants