Exported XML should contain version of abandon #112

z4f1r0v · 2016-10-23T21:40:07Z

Closes #105
Update sbt to incorporate sbt-buildinfo plugin
Move generated BuildInfo.scala to co.uproot.abandon
Add version xml elements to Journal and Balance exports with the version from BuildInfo.scala

Move generated BuildInfo.scala to co.uproot.abandon Add version xml elements to Journal and Balance exports with the version from BuildInfo.scala

z4f1r0v · 2016-10-23T21:44:17Z

HI @hrj! I did some work on #105. As you suggested I add the sbt-buildinfo to the general build.sbt of the project. Please take a look and let me know what you think.

hrj

Nice work @alexanderzafirov, thanks!

Have a couple of comments.

hrj · 2016-10-24T02:28:35Z

base/src/main/scala/co/uproot/abandon/BuildInfo.scala

+
+import scala.Predef._
+
+/** This object was generated by sbt-buildinfo. */


@alexanderzafirov This generated file should not be checked in to the repository.

I'm a bit confused. How are we going to reference the file when it is not being tracked by Git?

Isn't it generated automatically on compilation? I thought that is how the plugin worked but I maybe wrong.

It is. The problem is that I manually moved it from the main target folder to where it resides now.

Umm.. the point of this issue is to automate the version numbering. Developers will only update the sbt file when the version number increases, and the application will automatically pick up the version number when emitting to the XML file, etc.

If there is any manual step, then it defeats the purpose.

I suggest you verify whether the manual step is necessary.

Understood.

hrj · 2016-10-24T02:30:07Z

base/src/main/scala/co/uproot/abandon/Report.scala

@@ -252,6 +252,9 @@ object Reports {

  def xmlBalanceExport(state: AppState, exportSettings: XmlExportSettings): xml.Node = {
    <abandon>
+      <version>
+        { BuildInfo.version }
+      </version>


Would be better to keep it on one line and no extra spaces, for example:

<version>{BuildInfo.version}</version>

Ok. I just followed the "convention" from the lines below.

Can we keep the spaces. For readability's sake?

If it's a leaf node, then it is better to not have spaces. It will be easy for the application which is reading the XML file to parse the text content.

Actually, you could even make version an attribute of the abandon node. No need for a separate node.

If a node is a "structural node" with child nodes, then spacing and indentation makes sense.

Oh, sorry, I didn't realize that the spaces in the curly braces are actual spaces in the generated xml.
I actually though of landing version as an attribute to abandon but for some reason decided to be more straightforward.

I didn't realize that the spaces in the curly braces are actual spaces in the generated xml.

They are not; you can keep them. I thought you were referring to spaces between the XML tag and the scala code; those are significant; hence need to be removed.

Sorry for the confusion.

👍 for attribute.

hrj · 2016-10-24T02:30:39Z

base/src/main/scala/co/uproot/abandon/Report.scala

@@ -260,6 +263,9 @@ object Reports {

  def xmlJournalExport(state: AppState, exportSettings: XmlExportSettings): xml.Node = {
    <abandon>
+      <version>
+        { BuildInfo.version }
+      </version>


Same here: would be better to keep it on one line.

hrj · 2016-10-24T02:33:19Z

build.sbt

-    fork in run := true
+    fork in run := true,
+    buildInfoKeys := Seq[BuildInfoKey](name, version, scalaVersion, sbtVersion),
+    buildInfoPackage := "build.info"


By looking at the rest of the code, shouldn't buildInfoPackage be co.uproot.abandon?

I guess it makes sense. I just want to note that BuildInfo is generated in the target folder.

hrj · 2016-10-24T02:40:14Z

@alexanderzafirov the tests are failing. This is probably because they are checking for an exact match of the generated xml. We need to either:

change the expected output files to include the version (problem is, they will need to be changed with every release)
change the test code so that it ignores the version node
add a config option to not emit the version number in the emitted XML (just for testing).

I think last option (config property) is best.

z4f1r0v · 2016-10-24T07:26:06Z

Thanks for the comments, @hrj!
I agree that it is best to make this configurable. Just to be clear - you mean as a parameter to the methods?

hrj · 2016-10-24T07:34:35Z

I agree that it is best to make this configurable. Just to be clear - you mean as a parameter to the methods?

The CLI tests (which are failing) run the whole application. They don't call the internal methods directly.

So the parameter has to be either a command line option (passed via args to the main function) or as a property in the configuration file for that test. Either approach is fine with me, with a slight preference towards "command line option" because it will be easier.

z4f1r0v · 2016-10-24T07:36:54Z

Got it. I will implement the necessary fixes and ping you for review later!

Add new command line parameter Add parameter to differentiate between versioned xml and nonversioned one Start to fix tests Make client xml code compile

hrj · 2016-10-26T05:43:32Z

base/src/main/scala/co/uproot/abandon/Report.scala

-       </balance>
-    </abandon>
+  def xmlBalanceExport(state: AppState, exportSettings: XmlExportSettings, withVersion: Boolean): xml.Node = {
+    if (withVersion) {


To avoid duplicating the code, you can add the version attribute after creating the abandon node normally.

Example code to add an attribute to an Element:

def addAttrib(n: Elem, k: String, v: String) = n % new xml.UnprefixedAttribute(k, v, xml.Null)

Thanks @hrj. I was away for a couple of days and didn't want to have the changes laying around so I pushed them to the branch. I was definitely looking for a better implementation and your suggestion is great.

Looks good, except for the default.

thanks 👍

hrj · 2016-10-26T05:43:53Z

base/src/main/scala/co/uproot/abandon/Report.scala

-    </journal>
-   </abandon>
+  def xmlJournalExport(state: AppState, exportSettings: XmlExportSettings, withVersion: Boolean): xml.Node = {
+    if (withVersion) {


Same comment as before: avoid duplicating the code.

… element is created

hrj · 2016-11-01T07:00:29Z

base/src/main/scala/co/uproot/abandon/Config.scala

@@ -36,9 +36,10 @@ object SettingsHelper {
    val cliConf = new AbandonCLIConf(args)
    cliConf.verify()
    val configOpt = cliConf.config.toOption
+    val withVersion = cliConf.version.getOrElse(false)


The default here should be true I think.

That is, for normal usage, the version should be shown in the output. It is only the tests that have a problem with it, and hence they should explicitly disable it.

Set default value of versioned to be true Set the versioned value for cli test to false

coveralls · 2016-11-01T10:41:18Z

Coverage increased (+0.4%) to 67.977% when pulling 11090c9 on alexanderzafirov:master into b95601b on hrj:master.

z4f1r0v · 2016-11-01T10:44:16Z

@hrj Tests are happy now. I did a bit of rewording of the variables since the Boolean switches give you true if added. At the same time we want to have version added by default. Please let me know if you are happy with the outcome.

hrj

Awesome!

Update sbt to incorporate sbt-buildinfo plugin

ff69f38

Move generated BuildInfo.scala to co.uproot.abandon Add version xml elements to Journal and Balance exports with the version from BuildInfo.scala

hrj requested changes Oct 24, 2016

View reviewed changes

Delete BuildInfo

997f769

Add new command line parameter Add parameter to differentiate between versioned xml and nonversioned one Start to fix tests Make client xml code compile

hrj approved these changes Oct 26, 2016

View reviewed changes

hrj requested changes Oct 26, 2016

View reviewed changes

Alexander Zafirov added 2 commits October 31, 2016 16:06

Remove code duplication by adding the version attribute after the xml…

8524a99

… element is created

Add withVersion to ExportSettings

b557190

hrj approved these changes Nov 1, 2016

View reviewed changes

hrj requested changes Nov 1, 2016

View reviewed changes

Alexander Zafirov added 4 commits November 1, 2016 10:47

Give better name

88bf202

Set default value of versioned to be true Set the versioned value for cli test to false

Need two strings

784703f

Give proper naming due to scallop switch parameter

c62282b

Fix broken test

11090c9

hrj approved these changes Nov 3, 2016

View reviewed changes

hrj merged commit e3b1d40 into hrj:master Nov 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exported XML should contain version of abandon #112

Exported XML should contain version of abandon #112

z4f1r0v commented Oct 23, 2016 •

edited by hrj

Loading

z4f1r0v commented Oct 23, 2016

hrj left a comment

hrj Oct 24, 2016

z4f1r0v Oct 24, 2016

hrj Oct 24, 2016

z4f1r0v Oct 24, 2016

hrj Oct 24, 2016

z4f1r0v Oct 24, 2016

hrj Oct 24, 2016

z4f1r0v Oct 24, 2016

z4f1r0v Oct 24, 2016

hrj Oct 24, 2016

z4f1r0v Oct 24, 2016

hrj Oct 24, 2016

hrj Oct 24, 2016

z4f1r0v Oct 24, 2016

hrj Oct 24, 2016

z4f1r0v Oct 24, 2016

hrj commented Oct 24, 2016

z4f1r0v commented Oct 24, 2016

hrj commented Oct 24, 2016

z4f1r0v commented Oct 24, 2016 •

edited

Loading

hrj Oct 26, 2016

z4f1r0v Oct 31, 2016

hrj Nov 1, 2016

hrj Oct 26, 2016

hrj Nov 1, 2016

coveralls commented Nov 1, 2016

z4f1r0v commented Nov 1, 2016

hrj left a comment


		import scala.Predef._

		/** This object was generated by sbt-buildinfo. */

Exported XML should contain version of abandon #112

Exported XML should contain version of abandon #112

Conversation

z4f1r0v commented Oct 23, 2016 • edited by hrj Loading

z4f1r0v commented Oct 23, 2016

hrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrj commented Oct 24, 2016

z4f1r0v commented Oct 24, 2016

hrj commented Oct 24, 2016

z4f1r0v commented Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 1, 2016

z4f1r0v commented Nov 1, 2016

hrj left a comment

Choose a reason for hiding this comment

z4f1r0v commented Oct 23, 2016 •

edited by hrj

Loading

z4f1r0v commented Oct 24, 2016 •

edited

Loading