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

Support override of args.filter #2681

Merged
merged 1 commit into from May 14, 2017

Conversation

Projects
None yet
2 participants
@robander
Member

robander commented May 9, 2017

This request builds on #2637 to make the args.filter processing just a bit more flexible.

Currently (in 2.4 and with the update in #2637) the build-init task checks to see if a build defines the args.filter parameter. If not, it sets up a property that will skip all future ditaval processing. The problem here is that there is no way for a plugin to insert processing for default transforms before the build-init task; you either need a new transform with your own init step, or you can plug in before the bigger preprocess.

This request moves the check for args.filter into preprocess so that we do not set skip.ditaval.preprocess until it's needed. I see three benefits for this:

  1. A plugin can supply one or more common ditaval files. By inserting a target before preprocess, it can set args.filter to use those common properties for any or all existing transform types.
  2. A plugin can use its own property with a custom platform-independent separator, and then convert that property to an args.filter value that is appropriate for the system running the build. This is probably how I will handle the platform questions that came up after #2637 was merged -- use a property like custom.args.filter that always uses a comma or semi-colon, and then converts that separator to ; or : in args.filter.
  3. Even if those use cases are not broadly applicable, it's just a better design to avoid setting skip.ditaval.preprocess until we actually need it.

Signed-off-by: Robert D Anderson robander@us.ibm.com

@robander robander self-assigned this May 9, 2017

@robander

This comment has been minimized.

Show comment
Hide comment
@robander

robander May 9, 2017

Member

This picks up a change in the parameter passed to Java:
<param name="ditaval" value="${args.filter}" if="args.filter"/>

The attribute @location switched to @value. I'm not sure which is preferred here, but it's not a single location so that seemed incorrect.

Member

robander commented May 9, 2017

This picks up a change in the parameter passed to Java:
<param name="ditaval" value="${args.filter}" if="args.filter"/>

The attribute @location switched to @value. I'm not sure which is preferred here, but it's not a single location so that seemed incorrect.

@robander robander requested a review from jelovirt May 9, 2017

Support override of args.filter
Signed-off-by: Robert D Anderson <robander@us.ibm.com>
@robander

This comment has been minimized.

Show comment
Hide comment
@robander

robander May 10, 2017

Member

Per code review comments from @jelovirt -- there was no need for the property originally set up to allow skipping the merge process. Instead, making use of the if:set attribute suggested in #2675 to simplify the merge code.

Member

robander commented May 10, 2017

Per code review comments from @jelovirt -- there was no need for the property originally set up to allow skipping the merge process. Instead, making use of the if:set attribute suggested in #2675 to simplify the merge code.

@jelovirt

This comment has been minimized.

Show comment
Hide comment
@jelovirt

jelovirt May 11, 2017

Member

@robander My question was really about whether we should provide skip functionality at all. Now it's implemented with @if:set, but what's the use case for skipping the the merge? Why would a user want to skip it? The module will do nothing when there's no filter defined and the cost of creating the module and then immediately returning is very small.

Member

jelovirt commented May 11, 2017

@robander My question was really about whether we should provide skip functionality at all. Now it's implemented with @if:set, but what's the use case for skipping the the merge? Why would a user want to skip it? The module will do nothing when there's no filter defined and the cost of creating the module and then immediately returning is very small.

@robander

This comment has been minimized.

Show comment
Hide comment
@robander

robander May 11, 2017

Member

Eh, seemed worth avoiding the very small cost, but I agree it's very small. My thought was -- this module has a very specific purpose, which won't ever do anything when args.filter isn't set, so why run the module if it's not set?

I don't have a strong opinion either way.

Member

robander commented May 11, 2017

Eh, seemed worth avoiding the very small cost, but I agree it's very small. My thought was -- this module has a very specific purpose, which won't ever do anything when args.filter isn't set, so why run the module if it's not set?

I don't have a strong opinion either way.

@jelovirt jelovirt added this to the 2.5 milestone May 14, 2017

@jelovirt jelovirt merged commit 99fe7c2 into dita-ot:develop May 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@robander robander deleted the robander:feature/ditavaltweak branch May 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment