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

Improve handling of extension attributes from running steps #4

Closed
ndw opened this Issue Sep 5, 2011 · 6 comments

Comments

Projects
None yet
2 participants
@ndw
Owner

ndw commented Sep 5, 2011

Florent wrote: http://markmail.org/message/ivfpsyvyb3hxinle

* Made sure extension attributes are processed on all
  elements; added hook for an XStep to get access to the
  underlying Step for access to extension attributes on
  descendants.

Thanks. That solves the problem for XSteps. But most of the
steps are not XSteps, they are XProcSteps. And in Calabash those
are even all extending DefaultStep. So they have an
XAtomicStep (which is an XStep), but they are not an XStep
themselves.

I guess a simple and convenient solution is to add a method
like the following to DefaultStep, in order for extension code
manipulating steps to access the underlying XStep and thus the
extension attribtues:

public XAtomicStep getUnderlyingStep() {
    return step;
}

or preferably a more dedicated one like:

public String getExtensionAttribute(QName name) {
    return step == null
        ? null
        : step.getExtensionAttribute(name);
}
@ndw

This comment has been minimized.

Show comment
Hide comment
@ndw

ndw Sep 8, 2011

Owner

DefaultStep's already have access to the XAtomicStep in the 'step' field. And there's already a getExtensionAttribute() method that will return the extension attributes on the step.

I added getStep() to the XAtomicStep so that you could say:

Input input = step.getStep().getInput("source")
String foo = input.getExtensionAttribute(_ext_qname)

Without the ability to get at the underlying step, you couldn't get the Input object to get the extension attributes off of it.

So I think we're good. Reopen if you disagree.

Owner

ndw commented Sep 8, 2011

DefaultStep's already have access to the XAtomicStep in the 'step' field. And there's already a getExtensionAttribute() method that will return the extension attributes on the step.

I added getStep() to the XAtomicStep so that you could say:

Input input = step.getStep().getInput("source")
String foo = input.getExtensionAttribute(_ext_qname)

Without the ability to get at the underlying step, you couldn't get the Input object to get the extension attributes off of it.

So I think we're good. Reopen if you disagree.

@ndw ndw closed this Sep 8, 2011

@fgeorges

This comment has been minimized.

Show comment
Hide comment
@fgeorges

fgeorges Sep 8, 2011

But that code is legal from within the class deriving from DefaultStep, not from external code using such a step, right?

So yes, Load could access the extension attributes, but the goal is to access them from code outside of Calabash code base. A method receiving a Load object (e.g. XMLCalabashConfigurer.loadDocument(Load)) cannot access those attributes, because it does not have access to the protected member 'step'.

That's why I suggested to provide the underlying step, or probably more sensible only the extension attributes, through a new method on DefaultStep.

Did I miss something?

PS: I cannot reopen the issue, perhaps because I hadn't create it myself?

fgeorges commented Sep 8, 2011

But that code is legal from within the class deriving from DefaultStep, not from external code using such a step, right?

So yes, Load could access the extension attributes, but the goal is to access them from code outside of Calabash code base. A method receiving a Load object (e.g. XMLCalabashConfigurer.loadDocument(Load)) cannot access those attributes, because it does not have access to the protected member 'step'.

That's why I suggested to provide the underlying step, or probably more sensible only the extension attributes, through a new method on DefaultStep.

Did I miss something?

PS: I cannot reopen the issue, perhaps because I hadn't create it myself?

@ndw ndw reopened this Sep 8, 2011

@ndw

This comment has been minimized.

Show comment
Hide comment
@ndw

ndw Sep 8, 2011

Owner

Thanks for the clarification. Now I think I see the problem. I'll take a closer look.

Owner

ndw commented Sep 8, 2011

Thanks for the clarification. Now I think I see the problem. I'll take a closer look.

@fgeorges

This comment has been minimized.

Show comment
Hide comment
@fgeorges

fgeorges Sep 8, 2011

By the way, sorry I haven't think about it before, but if you
want an example of use, you can have a look at:

http://code.google.com/p/expath-pkg/source/browse/trunk/calabash/pkg-calabash/src/org/expath/pkg/calabash/PkgCalabashConfigurer.java

Look at the method loadDocument(Load step). For now, it is
commented out, but you can still see how I used a modified Load
with an extra getUnderlyingStep() method, returning the Load's
XAtomicStep. Probably creating a new getExtensionAttribute() is
a better idea though (instead of returning the whole underlying
step, uncontrolled).

fgeorges commented Sep 8, 2011

By the way, sorry I haven't think about it before, but if you
want an example of use, you can have a look at:

http://code.google.com/p/expath-pkg/source/browse/trunk/calabash/pkg-calabash/src/org/expath/pkg/calabash/PkgCalabashConfigurer.java

Look at the method loadDocument(Load step). For now, it is
commented out, but you can still see how I used a modified Load
with an extra getUnderlyingStep() method, returning the Load's
XAtomicStep. Probably creating a new getExtensionAttribute() is
a better idea though (instead of returning the whole underlying
step, uncontrolled).

@ndw

This comment has been minimized.

Show comment
Hide comment
@ndw

ndw Sep 11, 2011

Owner

Well. Meh. In the short term, I simply added a getStep() method to return the underlying XAtomicStep. I don't think that's a good answer, really, but I'm not sure I want to try to do more right now.

Owner

ndw commented Sep 11, 2011

Well. Meh. In the short term, I simply added a getStep() method to return the underlying XAtomicStep. I don't think that's a good answer, really, but I'm not sure I want to try to do more right now.

@ndw ndw closed this Sep 11, 2011

@fgeorges

This comment has been minimized.

Show comment
Hide comment
@fgeorges

fgeorges Sep 12, 2011

Good enough for me, it solves my problem. Now, the EXPath packaging is implemented completely, entirely in user space (w/o a custom build of Calabash) Thanks!

Good enough for me, it solves my problem. Now, the EXPath packaging is implemented completely, entirely in user space (w/o a custom build of Calabash) Thanks!

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