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

Rework data variable access in PreconditionContext #1360

Closed
leonard84 opened this issue Jul 16, 2021 · 5 comments
Closed

Rework data variable access in PreconditionContext #1360

leonard84 opened this issue Jul 16, 2021 · 5 comments

Comments

@leonard84
Copy link
Member

Context

In #1094 we added support for data variable access in the PreconditionContext the way it works currently is that it first tries to execute the condition without instantiating the instance/data variables, and when the condition fails with a MissingPropertyException we check if the propertyName matches a dataVariable and if that is the case we install an interceptor for the FeatureMethod and let it run. Then when feature method is invoked with the data variables, we execute the condition again.

Currently, we use propertyMissing and check against a Map that contains the data variables and if it has a matching entry we return it. That approach works but has some drawbacks, namely that it conflicts with the existing properties (os, sys, env, jvm, javaVersion) on the PreconditionContext, so if you have a data variable with the same name you can't access it.

The other downside, is that we only know the available dataVariables on the feature level, so with the current implementation we can't easily detect a valid data variable access.

In #1204 we also added support for instance that behaves similar to the data variables, it also uses MissingPropertyException, but it is resolved after the data variables are resolved.

#1359 will add support for shared, that will give access to shared fields when used on the Specification level. Currently, this also resolves after the dataVariables.

Proposed alternatives

  1. Introduce new data property, that exposes the data variables, removing the need for propertyMissing. instance and shared would also become real properties. If these properties are accessed while unset (static context) we will throw a dedicated exception with which we can easily decide to add the interceptors, reducing the complexity in the exception handling. This would be a breaking change, for those that already use the data variable access, but it would make the API more straight forward.
  2. Add data like in 1. but keep the existing propertyMissing handling for data variables, but give priority to data, instance, shared. This would mostly be backwards compatible, but could lead to confusing corner cases.
  3. Keep the current implementation.
@leonard84
Copy link
Member Author

leonard84 commented Jul 16, 2021

I prefer 1. because it makes the API more clear, as it cleanly separates access to the different sources, instead of mixing the data variables in with confusing precedence issues for the user. Furthermore, the current data variables also shadow static fields, as they are resolved before the static fields.
I would also consider the data variable access feature as @Beta although we couldn't annotate the part.

@leonard84
Copy link
Member Author

@spockframework/supporter please comment

@marcphilipp
Copy link
Member

I would also prefer (1) since it would make things much clearer. 👍

@masooh
Copy link

masooh commented Jul 19, 2021

I'm also in favor of 1:

  • easier to understand the condition as it is clear where to look for
  • gives the opportunity for clear exception messages
  • gives the opportunity for specifc code completion

@mkutz
Copy link
Contributor

mkutz commented Aug 2, 2021

I don't see any value in 2 and 3 has obvious drawbacks, even though it never bothered me personally in the past.
So I'm in favor of 1 as well.

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

No branches or pull requests

4 participants