Skip to content

Invocation fix (Close #95) #99

Merged
merged 9 commits into from Oct 19, 2012

2 participants

@ahawkins

The issue is a complex issue caused by a series of small problems. All the small issues are described in detail in #95. I've tried to structure the commits so that each one addresses a problem.

twinturbo added some commits Oct 15, 2012
twinturbo Add Pipeline#fingerprint d70251e
twinturbo Fix already initialized constant warnings 620cb7b
twinturbo Temp directory naming is an instance concern 68e1b5a
twinturbo #copy assigns project correctly aa4937c
twinturbo Remove #last_manifest
This commit is the first step in fixing some internal logic errors.
The code previously used two manifests. These were both the same
file even though the last manifest was only ever read and never written.
The current manifest was never read and only written. This caused
problems. This commit changes DynamicFileTask to use only one manifest.

It also changes DynamicFileTask to be more resilient to internal
mistakes. This commit assures that a manifest is assigned to a
DynamicFileTask instance. Previously empty manifests were created as
needed. This was a problem because the proper manifest was not being
assigned correctly. This commit also changes DynamicFileTask#needed? to
raise an error when there is no manifest. This ensures that this
implementation error will never happen. It does make tests more verbose,
but I think the change is worth it
7935a4a
twinturbo Fix test failures introduced by previous commit c341870
twinturbo Refactor code to reduce manifest passing
This commit introduces Filer#manifest which returns the set manifest or
the pipeline's manifest. This makes testing easier because you can use
Filer#manifest= to set on for that test. Pipeline#manifest delegates to
the project.

I think there is still problems with this code. The connection between
Project, Pipeline, Filter, and DynamicFileTask is too complicated. This is
especially annoying because DynamicFileTask requires a manifest to
function properly. However, the manifest must be passed down from the
project (then through a pipeline and into a filter) which builds the
actual DynamicFileTask instances. I think this is a code smell.
Introducing more collaboraters is making the tests more complex. Using a
singleton is one possible solution. I will think about this later.
ec573df
twinturbo Load and save the manifest around every invocation
This ensures that the manifest is always correct across multiple
invocations and processes
bc1dd86
twinturbo Test new files are only generated if required f2355b7
@wycats wycats merged commit f2355b7 into livingsocial:master Oct 19, 2012

1 check failed

Details default The Travis build failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.