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

BUPH-146 | add support for source override behavior #84

Open
wants to merge 1 commit into
base: 1.x
Choose a base branch
from

Conversation

jpmarcotte
Copy link
Contributor

@jpmarcotte jpmarcotte commented Dec 7, 2022

This is just for the 1.x branch.

Ticket Description:
If a file getting put into fab already exists in src, then Buphalo skips it. This can make testing the creation of new templates difficult. It also can create cases where it was intended to create a new file with buphalo, but not removing the source file resulted in no net changes.

This could be handled by an env var that has one of 3 values:

SourceOverrideBehavior (what to do if an override is found in the source directory):

  • skip (do not write the file; this is the existing default behavior)
  • write (write the file anyway; this can be useful for testing and creating new templates)
  • throw (throw an exception; this is a safer behavior that forces unambiguous intention)

Copy link

@dreinhuber dreinhuber left a comment

Choose a reason for hiding this comment

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

The logic of the source override behavior implementation looks good/makes sense to me. I think I have some understanding of how the tests are written as well.

I assume since we have a default, this means that the change would be backwards compatible and wouldn't require existing Buphalo setups to include the variable in order to work?

@@ -1,7 +1,11 @@
parameters:
env(Neighborhoods_Buphalo_V1_Actor_WriterInterface__SourceOverrideBehavior): !php/const \Neighborhoods\Buphalo\V1\Actor\WriterInterface::SOURCE_OVERRIDE_BEHAVIOR_DEFAULT
Copy link

@dreinhuber dreinhuber Dec 21, 2022

Choose a reason for hiding this comment

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

I take it this ensures the default is always being used and line 3 then overrides the default if a variable is provided?

Since it doesn't show in the conversation view, line 3 is:

Neighborhoods\Buphalo\V1\Actor\WriterInterface.SourceOverrideBehavior: '%env(Neighborhoods_Buphalo_V1_Actor_WriterInterface__SourceOverrideBehavior)%'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. It's not a super-intuitive notation, but as far as I can tell, this is how Symfony handles defaults for environment variables that might not be set. And it makes it so that the code in Buphalo doesn't really have to rely on a "default" and can instead be explicit and know the value will be set.

@jpmarcotte
Copy link
Contributor Author

This should be good to go, I just never got around to cutting a release (I was leaving it open here until I was ready to actually test it in ListingService). The logic also needs to be implemented for the 2.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants