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

pipe_tasks tickets/DM-2191, review 2 #8

Merged
merged 5 commits into from Jun 3, 2015
Merged

Conversation

TallJimbo
Copy link
Member

A fresh PR for a new review after John's refactoring.

@TallJimbo
Copy link
Member Author

Looks like you need a bin/ script for coadd measurement as well.

"""
ConfigClass = TransformConfig
_DefaultName = "transform"

def __init__(self, measConfig, pluginRegistry, inputSchema, *args, **kwargs):
def __init__(self, measConfig, pluginRegistry, inputSchema, outputType, *args, **kwargs):
Copy link
Member Author

Choose a reason for hiding this comment

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

You can actually get the PluginRegistry from measConfig: measConfig.plugins.registry, so it doesn't need to be a separate argument.

Also, can't comment on a few lines below, since it's not a change, but "measurments" is a typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not realise this -- thanks. It makes the code significantly simpler. Great!

Rather than hard-coding the use of 'src', we provide a function that enables
users to build their own tranformation tasks for arbitrary source type names.

Note that we can't simply set the source type through configuration, as
configuration is persisted and (broadly) immutable, whereas we may want to
transform multiple different source types in the same repository.
This is a nice simplification to the logic, and was (in fact) possible all
along. Thanks to Jim Bosch for pointing it out.
@jdswinbank jdswinbank merged commit c98b0e6 into master Jun 3, 2015
@ktlim ktlim deleted the tickets/DM-2191 branch August 25, 2018 06:45
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

2 participants