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

Filter out DataBinding expressions when extracting resources from xml files #543

Merged
merged 2 commits into from
Jan 5, 2022
Merged

Filter out DataBinding expressions when extracting resources from xml files #543

merged 2 commits into from
Jan 5, 2022

Conversation

nsk-mironov
Copy link
Contributor

@nsk-mironov nsk-mironov commented Jan 4, 2022

I've tried running buildHealth with 0.79.0 version of the plugin and got the same exception as in #531 and #542. The only difference that it has nothing to do with @null in my case and is actually caused by data binding expressions like binding:subtitle="@{model.subtitle}".

The fix from #534 actually prevents the exception in my case, but it doesn't cover all databinding related scenarios. For example, expressions like binding:outlineRadius="@{@dimen/radius_normal}" are still parsed as a valid AttrRef(type = "{@dimen", id = "radius_normal}") object.

This PR introduces the following changes:

  • AttrRef.from now always returns null for all data binding expressions.
  • ResSpec.groovy runs one more case which is specially dedicated to data binding expressions.

Copy link
Owner

@autonomousapps autonomousapps left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! Very much appreciated. Only one change I'd like to see.


import com.autonomousapps.advice.Advice

class DataBindingWithExpressionsProject(
Copy link
Owner

Choose a reason for hiding this comment

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

Please rewrite this fixture to use a similar style to AttrResWithNullProject from the spec just above your new one. This is the first style of fixture used in this project and... I hate it. I'd like to eliminate it entirely at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, not a problem. Please check my latest commit with an updated DataBindingWithExpressionsProject implementation.

*
* Will return `null` if the map entry doesn't match an expected pattern.
*/
fun from(mapEntry: Map.Entry<String, String>): AttrRef? {
if (mapEntry.isId()) return null
if (mapEntry.isToolsAttr()) return null
if (mapEntry.isDataBindingExpression()) return null
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for updating this in both locations!

Copy link
Owner

@autonomousapps autonomousapps left a comment

Choose a reason for hiding this comment

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

LGTM!

@autonomousapps autonomousapps merged commit 4792f4b into autonomousapps:main Jan 5, 2022
@nsk-mironov nsk-mironov deleted the databinding-expressions-handling branch January 5, 2022 09:20
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.

2 participants