Skip to content

Add support to set object properties via XML#765

Merged
ewanharris merged 8 commits intotidev:masterfrom
brentonhouse:feature-xml-attribute-prefix
Apr 1, 2019
Merged

Add support to set object properties via XML#765
ewanharris merged 8 commits intotidev:masterfrom
brentonhouse:feature-xml-attribute-prefix

Conversation

@brentonhouse
Copy link
Copy Markdown
Contributor

@FokkeZB
Copy link
Copy Markdown
Contributor

FokkeZB commented Mar 3, 2016

Great work! I guess we need a test app added @feons ?

@feons
Copy link
Copy Markdown
Contributor

feons commented Mar 3, 2016

Yup! Please include a test app for this PR. Thank you!

@brentonhouse
Copy link
Copy Markdown
Contributor Author

I haven't done one of the test apps for Alloy before but I will take a look at what it takes to create one. Thanks!

@feons
Copy link
Copy Markdown
Contributor

feons commented Mar 3, 2016

@brentonhouse, to get started you can use this command in your Alloy repo node tools/create_test.js ALOY-### where ALOY-### is the ticket number. That'll create a folder under test/apps/testing/ALOY-###, you can fill in code necessary to test the PR.

@FokkeZB
Copy link
Copy Markdown
Contributor

FokkeZB commented Mar 3, 2016

Then use run the test to verify it works and run generate and verify the generated code is as it should be. Submit including the generated code and done.

man.. we need to document this better

@brentonhouse
Copy link
Copy Markdown
Contributor Author

@skypanther
Copy link
Copy Markdown
Contributor

@FokkeZB the create_test.js file is minimally documented at https://github.com/appcelerator/alloy/tree/master/tools and I think I had added stuff to our internal wiki about it. But, I agree that it should be better documented for contributors.

@hansemannn
Copy link
Copy Markdown
Contributor

@feons @brentonhouse This should go into 2.0.0, can we schedule this?

@hansemannn hansemannn added this to the 2.0.0 milestone Dec 7, 2017
@hansemannn hansemannn removed this from the 2.0.0 milestone Aug 10, 2018
@hansemannn
Copy link
Copy Markdown
Contributor

@feons Please review. This can even go pre 2.0.0 since I do not see a breaking change here.

@hansemannn
Copy link
Copy Markdown
Contributor

@ewanharris @brentonhouse Ping! Can this please be reviewed? Highly demanded feature.

Copy link
Copy Markdown
Member

@ewanharris ewanharris left a comment

Choose a reason for hiding this comment

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

Similar to #765, I think it'd be best to do this in the initial pass in getParserArgs. Before we set the the attribute on createArgs here, we could maybe check if it's a dotted property and then split the property and key name to be set on createArgs?

@build
Copy link
Copy Markdown

build commented Mar 29, 2019

Warnings
⚠️

Please ensure to add a changelog entry for your changes. Edit the CHANGELOG.md file and add your change under the Unreleased items header

Messages
📖

✅ All tests are passing
Nice one! All 3480 tests are passing.

Generated by 🚫 dangerJS against 33e9410

Copy link
Copy Markdown
Member

@ewanharris ewanharris left a comment

Choose a reason for hiding this comment

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

The changes look good to me!

Merging

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants