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

fix build in jruby && truffleruby #265

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix build in jruby && truffleruby #265

wants to merge 5 commits into from

Conversation

seuros
Copy link
Member

@seuros seuros commented Jan 1, 2023

Removed all deprecation warnings.
Green in Truffleruby
2 specs failing in jruby (i will fix them later)
All green in Ruby
Updated syntax to make it work with the other engines.

@seuros seuros force-pushed the testexotic branch 2 times, most recently from c99d58d to d117199 Compare January 1, 2023 18:30
@seuros seuros changed the title fix build in jruby fix build in jruby && truffleruby Jan 1, 2023
@seuros seuros self-assigned this Jan 1, 2023
@seuros seuros marked this pull request as ready for review January 1, 2023 23:32

property :qualification

property :name, namespace: "http://eric.van-der-vlist.com/ns/person"
Copy link
Member

Choose a reason for hiding this comment

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

Why change this line ? Shouldn't we check both namespace as method and namespace as option ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Namespace already inherited from parent object.

It the same

Copy link
Member

@yogeshjain999 yogeshjain999 Jan 5, 2023

Choose a reason for hiding this comment

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

No, I think the namespace defined at property level overrides the parent one. For example, character.qualification remains lib where as character.name changes to hr. We should revert this change

Copy link
Member Author

Choose a reason for hiding this comment

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

The namespace change need to have similar structure. we can fix this test when i will fix the jruby build.

Copy link
Member

@yogeshjain999 yogeshjain999 Jan 5, 2023

Choose a reason for hiding this comment

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

Ok, I think let's skip this for jruby and we'll come up with different example so it can be compatible to test overriding the namespace.

For now, let's revert this change.

@@ -11,27 +11,27 @@
# <hr:born>1922-11-26</hr:born>
# <hr:dead>2000-02-12</hr:dead>
# </hr:author>
# <lib:character id="PP">
# <hr:character id="PP">
Copy link
Member

Choose a reason for hiding this comment

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

Why change from lib to hr ?

Copy link
Member Author

Choose a reason for hiding this comment

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

xmlns:lib="http://eric.van-der-vlist.com/ns/library"
xmlns:hr="http://eric.van-der-vlist.com/ns/person">

Lib is for library and HR for the person

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but character is the property of lib and not hr

Copy link
Member Author

Choose a reason for hiding this comment

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

It possible in the code, but not valid xml, that is why it was failing in jruby since the library in java don't support it.

I can skip this test in jruby

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