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

Sd 21 #57

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

Sd 21 #57

wants to merge 9 commits into from

Conversation

rbattenfeld
Copy link

Hi Andrew and Aslak

I implemented a first version of descriptor readers. I tried not to change the existing descriptors. The changes are:

  1. The read-only descriptors are called DescriptorReader, e.g. WebAppDescriptorReader
  2. The descriptor readers are type safe as the mutual descriptors but I couldn't reuse the same types. I think this is ok. A user knows in advance which descriptors (mutual or read-only).
  3. I removed all service files. These files are now generated. This is possible because the metadata-parser allows also to specify the descriptor name.
  4. I wrote a small test case in the ejb-jar31 test folder to check the basics.

Let me know what you think.

Happy Easter
Ralf

@ALRubinger
Copy link
Member

Awesome to see some progress on this. Some comments:

  1. Work to continue synchronized with upstream/SHRINKDESC-21
  2. CI Job: https://shrinkwrap.ci.cloudbees.com/job/ShrinkWrap_Descriptors_upstream-SHRINKDESC-21/
  3. I'd been expecting the mutable view to extend from the immutable
    one. In other words: EjbJar31MutableDescriptor extends
    EjbJar31Descriptor.
  4. The patch looks like a lot of reformatting is taking place, which
    makes it especially difficult in the generated code scenario to see
    what's really changed. Can we back out the refomatting so that the
    diffs are more true to the functionally-changed content?

This can fuel our discussions now set for:
http://timeanddate.com/worldclock/meetingdetails.html?year=2012&month=4&day=9&hour=17&min=0&sec=0&p1=195&p2=197

@rbattenfeld
Copy link
Author

Hi Andrew,
I reformatted the ddJavaAll.xsd hoping that the this is now better.

Cheers,
Ralf

@ALRubinger
Copy link
Member

Feedback sought to move forward: https://community.jboss.org/thread/198374

@rbattenfeld
Copy link
Author

Hi Andrew

I synced my stuff with yours. I will continue with the API enhancements because this will makes it easier. What I will do is:

  • allow to define several arguments in one method like envEntry(name, type, value) in contrast to .name().type().value()
  • thinking to get rid of the not so popular up() method. So, the parent is hopefully not required anymore

Already done is to pass a 'class' as argument.

Regards,
Ralf

@ALRubinger
Copy link
Member

Cool, I see that this is rebased of upstream/SHRINKDESC-21.

Some questions:

  1. Why the source changes to metadata-parser? Should this be part of
    another issue? The end goal is to change the generation strategy
    sure, but first we need to agree on the final form in the API
    prototyping being done.
  2. If we're making an object model to represent hierarchical data, how
    would we do this without "up()"?

With those addressed, we can likely squash your commits into one and
put it into upstream/SHRINKDESC-21.

Looking forward to making even more progress over this in person at
JUDCon and JBoss World!

S,
ALR

@rbattenfeld
Copy link
Author

Sorry that I was not precise enough. API changes must be agreed. I just want to discover possible solutions for problems reported by others. So, the existing API didn't change at all.

Question 1: These changes should allow that sequences of elements within a class can be passed in one call. When I mean sequences then I mean xsd:sequences which clearly says 'enter the following arguments in the this order'.

Question 2: The only other solution for up() is perhaps the standard new() operator. I think JAXB does it in the same way. Again, this is just an idea.

I will certainly not change the API without agreement and will also work on the read-only feature.

I hope this clarifies a little bit. And yes, I am looking forward as well to make progress at JUDCon and JBoss World.
Ralf

@rbattenfeld
Copy link
Author

I investigated some possible API enhancements which can be realized out of the generation step. The changes are just add-ons. The existing API is not changed at all. I will write a comment in the forum about this,

I will now investigate the read-only aspect further, so that we have a good base next week for a deeper discussion.

Regards,
Ralf

@rbattenfeld
Copy link
Author

Hi Andrew

I synced your POC. I am not so happy because I realized that the idea I had introduces a new layer. The current state is:

  1. The descriptors are based on your POC
  2. The filter element in your POC is replaced by the root element because this is the only valid element that can have a ref to the descriptor.
  3. The other elements are as before. I didn't had time to go further in my holidays:-)

I think, the state is good enough for the discussion.

See you tomorrow!

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.

3 participants