Skip to content

Adds CLI and incrementVersion#21

Closed
FokkeZB wants to merge 3 commits intotonylukasavage:masterfrom
FokkeZB:develop
Closed

Adds CLI and incrementVersion#21
FokkeZB wants to merge 3 commits intotonylukasavage:masterfrom
FokkeZB:develop

Conversation

@FokkeZB
Copy link
Copy Markdown

@FokkeZB FokkeZB commented Sep 30, 2014

  • Updates test
  • Updates fixture tiapp.xml to have a semantic version as well as a android:versionName and android:versionCode
  • Updates README
$ tiapp version minor

- Updated test
- Updated fixture to have semver
- Increments android/manifest[android:versionCode] as well
- Removes android/manifest[android:versionName] if found
- Updates README
- Adds tests
@tonylukasavage
Copy link
Copy Markdown
Owner

A couple questions/concerns:

  • Lumping together CLI and the new API in a single PR makes this harder to evaluate.
  • What's the use case for the CLI? I really don't want to add a CLI only to support this version functionality. If there's going to be a CLI (which I'm not sure we need in this project), it needs to support the full API. I'll need a little more convincing here that this will practically be used as a CLI.
  • I have major issues with incrementVersion implementation and design
    • Why are you doing all the version handling by hand instead of using something like semver?
    • This will all break when using CI installs that don't fit the X.Y.Z version pattern.
    • The name itself doesn't fit at all with the existing API. it would make more sense as something like setVersion(version), forgetting the major/minor/patch stuff.

In the end, I need more convincing that the CLI is necessary (and it'll need more work and its own PR if we agree) and I don't think the incrementVersion function is something that I want to put in the this API, particularly when you could do it simply with semver in conjunction with the existing API.

@FokkeZB
Copy link
Copy Markdown
Author

FokkeZB commented Sep 30, 2014

I agree that:

  • The CLI should cover all of the API. Reason I did not is because I didn't have time to, even more since I knew I couldn't be sure you'd just accept it ;)
  • incrementVersion() is of a different abstraction level then the 1-1 API the module now offers.
  • It should use the semver module. Reasons I did not are two-fold:
    1. Apple does not support the full semver syntax, only m.m.p
    2. I took this from a Grunt script I already had ;)

I don't agree on:

  • It breaking CI installs. It will throw an error if the current version is not m.m.p and not change anything. It will be clear that if you want to use this method, you have to use semver.
  • The name doesn't fit. Yes, if you forget about the major/minor/patch it would be setVersion() but that's not what it is now. It increments, so the verb is increment, not set.

For the CLI the use case is clear. Simple tiapp.xml manipulation via the command line without digging into XML or having to write a script to use the module. I'd like to see:

$ tiapp add module nl.fokkezb.cool --platform iphone
$ tiapp name "My great app"
$ tiapp property ti.defaultunit dp --type string
...

@tonylukasavage
Copy link
Copy Markdown
Owner

As to your 2 counter-points:

  1. I don't want to include special version handling, particularly one that doesn't work in certain cases.
  2. I do not want to deviate from the get/set/remove verbiage for the API.

I'm going to close this, because as I also noted in #22, the version functionality changes seem better suited for a different module that uses tiapp.xml as a dependency. tiapp.xml will be only a direct 1 to 1 mapping of the API verbiage to operations on elements in the tiapp.xml file.

All that said, I's like to work with you on the CLI. I thin kthe idea you put forth here is a little short sighted, but I think I'm going to take this work as inspiration and start a new issue. I'd appreciate your feedback there on a CLI that would allow full access to the entire API.

@tonylukasavage tonylukasavage mentioned this pull request Oct 30, 2014
@FokkeZB FokkeZB deleted the develop branch November 27, 2014 15:38
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