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

SetAttrs should take Elem as argument #199

Merged
merged 6 commits into from
Apr 14, 2021
Merged

Conversation

pawelkaczor
Copy link
Contributor

No description provided.

@pawelkaczor pawelkaczor changed the title SetAttrs should take current attributes as argument SetAttrs should take Elem as argument Apr 4, 2021
@geirolz
Copy link
Owner

geirolz commented Apr 9, 2021

Hi @pawelkaczor thanks for your contribution! Let me check your proposal, could you please add more details and examples(so we can add also some documentation about it) in the meanwhile?

@geirolz geirolz self-assigned this Apr 9, 2021
@geirolz geirolz added enhancement New feature or request nice to have Feature proposal nice to have labels Apr 9, 2021
Copy link
Owner

@geirolz geirolz left a comment

Choose a reason for hiding this comment

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

Add documentation

@pawelkaczor
Copy link
Contributor Author

Hi @geirolz. I would rather treat this issue as a "missing feature". Imagine you just want to update a value of an existing attribute (like value = value + 1). How can I accomplish this with advxml?

@pawelkaczor
Copy link
Contributor Author

pawelkaczor commented Apr 9, 2021

Concrete example:

  private val dataAgeAttrAddition: ComposableXmlModifier =
    SetAttr(attrs => {
      val age = DataAge(Instant.parse(attrs \@ "CREATED_AT"), clock.instant)
      k"DATA_AGE" := age.asString
    })

@geirolz
Copy link
Owner

geirolz commented Apr 9, 2021

Hi @geirolz. I would rather treat this issue as a "missing feature". Imagine you just want to update a value of an existing attribute (like value = value + 1). How can I accomplish this with advxml?

Good point, I agree. It is achievable but with some workaround.
Does it introduce some breaking changes for the API? Sorry I have no time to see the code now, I will this evening

If it does we can introduce something new like UpdateAttrs action

@pawelkaczor
Copy link
Contributor Author

A new constructor method has been added to the API. This is not a breaking change.

@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #199 (aa296c6) into master (bde7c45) will decrease coverage by 0.31%.
The diff coverage is 50.00%.

❗ Current head aa296c6 differs from pull request most recent head 1f6d1bf. Consider uploading reports for the commit 1f6d1bf to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
- Coverage   99.42%   99.11%   -0.32%     
==========================================
  Files          21       21              
  Lines         347      338       -9     
==========================================
- Hits          345      335      -10     
- Misses          2        3       +1     
Impacted Files Coverage Δ
.../scala/advxml/instances/AllTransforInstances.scala 96.96% <50.00%> (-3.04%) ⬇️
...s/core/src/main/scala/advxml/core/data/Value.scala 96.29% <0.00%> (-0.14%) ⬇️
...ore/src/main/scala/advxml/core/data/KeyValue.scala 100.00% <0.00%> (ø)
...e/src/main/scala/advxml/syntax/AllDataSyntax.scala 100.00% <0.00%> (ø)
...src/main/scala/advxml/core/transform/XmlZoom.scala 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bde7c45...1f6d1bf. Read the comment docs.

@geirolz
Copy link
Owner

geirolz commented Apr 13, 2021

@pawelkaczor if you can add some tests for this then we can merge it!

Please check XmlTransformationSpec, TransformXmlRuleSyntaxTest and TransformModifiersTest

@pawelkaczor
Copy link
Contributor Author

Done. Not sure if the test fails because of my change?

@pawelkaczor
Copy link
Contributor Author

Looks like the tests were failing before my changes.

@geirolz
Copy link
Owner

geirolz commented Apr 14, 2021

Looks like the tests were failing before my changes.

Yes, no problem I'm going to fix it, one done I merge. Thanks again for the contribution!

@geirolz geirolz merged commit 1d3727a into geirolz:master Apr 14, 2021
@geirolz
Copy link
Owner

geirolz commented Apr 14, 2021

@pawelkaczor merged! Just added you as a contributor in the readme file, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request nice to have Feature proposal nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants