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

config-file element should support a src attribute #9

Open
wildabeast opened this issue Oct 17, 2012 · 10 comments
Open

config-file element should support a src attribute #9

wildabeast opened this issue Oct 17, 2012 · 10 comments

Comments

@wildabeast
Copy link

Right now a config-file element looks like

<config-file target="res/values/strings.xml" parent="/resources">
    <string name="app_picker_name">Applications</string>
</config-file>

would like to add support for sourcing this from a file:

<config-file targe="res/values/strings.xml" src="plugin-res/strings.xml" />

where plugin-res/strings.xml contains:

<resources>
    <string name="app_picker_name">Applications</string>
</resources>

So that if both a plugin and the target application have the same xml resource file, they get merged.

@alunny
Copy link
Owner

alunny commented Oct 18, 2012

cc @mreinstein @filmaj who are also likely to have opinions on this :)

@mreinstein
Copy link

I think it's a neat idea, there are pros and cons to doing this.
PROS:

  • can have very large blocks of XML that won't clutter up the manifest file
  • opens up the possibility of switching to a json based package format

CONS:

  • the configuration is now split among several files

If you decide to do this, I would make it non-optional (required) and switch the package format to json since node has a lot of tools for dealing with json.

Regardless of whether you decide to do this or not, I'd recommend changing the tag name from config-file to xml-graft.

@wildabeast
Copy link
Author

Regarding your CON, the particular use case is for an android plugin, specifically a long list of elements in the plugin's values/strings.xml file. I personally wouldn't even consider this 'configuration', its more like data. The configuration is only really split up if the developer decides to do so.

With this use case in mind I lean towards keeping it optional, as in this case, the developer might wish to keep configuration-specific items (such as android-manifest additions) in the plugin manifest, while separating out all of their view strings.

PS I like your json evangelism.

@mreinstein
Copy link

$statement = PS I like your json evangelism;

$new_statement = preg_replace('/evangel/g', $statement, 'pragmat'); # :)

Many people are suggesting using npm for all of the plugin registry operations. I don't agree with this, but if that is the path chosen, I don't think it's a good idea to have 2 manifest files that overlap (plugin.xml and package.json). Being able to maintain 1 package manifest is strongly encouraged. moving to json may enable this capability, since adding additional attributes to npms package.json shouldnt interfere with the operation of npm, and will allow data to be embedded that is pg/cordova install logic specific. That's why I'm favoring JSON, not for the sake of using it. Though having built-in suppport for json is some pretty sweet cake icing.

@alunny
Copy link
Owner

alunny commented Oct 18, 2012

@mreinstein: this is not a theoretical spec. We have working code running in production using this. It solves actual problems that we have building out our service. Any change needs a lot of justification.

Making every config change to an XML based file (and there can be lots on Android) reside in a different file that has to be merged (and merging two XML files, if we do that in full generality, is a non-trivial problem) is a lot of work, and provides no discernable benefit to plugin developers or end users.

If you want to have the JSON/XML discussion again, please open a different issue here.

@wildabeast I think we still need the parent XPath selector, so it would look something like:

<config-file targe="res/values/strings.xml" src="plugin-res/strings.xml" parent="/resources" />

Then the pseudo-code would look like:

var docOne = xml.parse('res/values/strings.xml'),
    docTwo = xml.parse('plugin-res/strings.xml'),
    parent = docOne.query('/resources'),
    newChildren = docTwo.queryall('/resources/*');

for (child in newChildren)
   parent.addChild(child);

@mreinstein
Copy link

@alunny I never implied it was a theoretical spec. I'm just explaining to you my take on it. You are specing out new behavior, and I'm trying to give you pros and cons to both approaches as I see them. If you don't want my opinion don't solicit it and then complain about my feedback. It's a waste of your time, and mine.

@alunny
Copy link
Owner

alunny commented Oct 18, 2012

Not complaining, just disagreeing. I did not intend to offend you, and I'm
sorry that I did.

My point, however clumsily stated, was that changes should be based on
maximum benefit with minimum effort, and that any extra-large effort
requires a suitably extra-large benefit. I do not think your suggestions
meet that criteria

On 18 October 2012 10:47, Mike Reinstein notifications@github.com wrote:

@alunny https://github.com/alunny I never implied it was a theoretical
spec. I'm just explaining to you my take on it. You are specing out new
behavior, and I'm trying to give you pros and cons to both approaches as I
see them. If you don't want my opinion don't solicit it and then complain
about my feedback. It's a waste of your time, and mine.


Reply to this email directly or view it on GitHubhttps://github.com//issues/9#issuecomment-9573836.

@filmaj
Copy link
Collaborator

filmaj commented Oct 18, 2012

src attribute for xml changes

If it's easier to manage a plugin, in your opinion, @wildabeast, by splitting out some config stuff in separate files, then I think adding this is fine. It's more of an option / just adds more flexibility. I do agree with @alunny that I would still keep the parent element attribute in there. What if you were to merge XML that has conflicting attributes? For example, with:

<resources someattrib="poop"></resources>

and:

<resources someattrib="crap"></resources>

How would this get merged?

Multiple config files

I think rolling with both a package.json and plugin.xml is fine at the moment. The json file would solve discoverability and versioning, basically, what npm solves well now. The plugin.xml would solve the plugin stuff. We can always explore merging the two but I don't think it is super high priority, personally.

With the two separate you could also author a plugin that will not be put into any central registry (a plugin without a package.json, for example), but has all the bits necessary (plugin.xml + directory structure) that pluginstall can use to do its thing.

@mreinstein
Copy link

@alunny try putting yourself in my position. I'm not on pg build team. I don't know about all the things that happen behind the scenes, and all the design decisions you've made. I'm not in on the face-to-face meetings you guys have, and I don't know about the exciting plans you guys probably have in store. I don't think it's fair to blanket label my discussion points as some kind of ivory tower academic view. I'm doing the best I can with the limited knowledge of what you and your team are doing.

I can give pros and cons on design decisions, given the limited context I have about how pluginstall works, and provide some suggestions about which one feels right to me, given what I know. And to be honest, I think I've been pretty good about trying to present technical issues from both sides, and encourage rationale discussion. That's about all I can offer at this point.

@wildabeast
Copy link
Author

@filmaj re: What if you were to merge XML that has conflicting attributes? Good question.

My initial thought is that elements with conflicting attributes don't get merged, they would be considered separate elements. This is due to a use case like:

doc1:

<platform name="ios">...</platform>

doc2:

<platform name="android">...</platform>

which definitely shouldn't get merged into one element.

The merging of xml docs definitely could get complicated. With xml being old as shit, I would think / hope this has been standardized. Will research.

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

No branches or pull requests

4 participants