Issue 2 and 3: Add documentation to Deposit and DepositProperties #22

lindareijnhoudt · 2018-08-03T08:27:00Z

fixes #3
fixes #2

Where should the reviewer @DANS-KNAW/easy start?

some questions in the code, as TODO's

lindareijnhoudt · 2018-08-03T08:28:51Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

@@ -367,6 +384,21 @@ object Deposit {
      } yield new Deposit(baseDir, bag, properties)
  }

+  //TODO wouldn't it be better to be able to provide the payloadDir and a baseDir for the Deposit separately?


Or as a separate method. I guess it could be used by easy-deposit-api to copy the deposit from the draft-area to the staging-area

no, a deposit is created in the draft-area and then moved/copied to the staging area. I don't see the need for a dans-bag-lib method for that. Just use better.files.File.move/copy

What I meant to question with this is: 'Would it make sense to have an extra method that doesn't create the bag in-place, but rather at a different location, indicated by baseDir?'. However, after thinking somewhat more about this, the user of this library can just do that by itself by first copying the original data to baseDir and then calling this method.

If you agree, feel free to remove this line.

@jo-pol and i thought to use such a method to automatically check the result of the copy by validating the checksums

to check if a Bag is complete/valid/virtually-valid, you can use the methods described in #1 (PR in #13), I guess.

Besides, what do you want to compare the checksums against? The manifest files will only be calculated during the execution of this method. Hence, I really would not like to do that twice within one method... Of course I'm perfectly fine with comparing these calculated checksums in the manifests with the checksums of the original files (where the files where copied from, provided they weren't moved rather than copied), but that would probably something to be taken care of by the user of the library, as that is the one that is gonna do the copying/moving anyway, right?

lindareijnhoudt · 2018-08-03T08:30:19Z

src/main/scala/nl/knaw/dans/bag/DepositProperties.scala

@@ -39,6 +39,12 @@ case class DepositProperties(creation: Creation = Creation(),
                             springfield: Springfield = Springfield(),
                             staged: Staged = Staged()) {

+  //TODO shouldn't this always be 'deposit.properties'? in the current Deposit.baseDir?


or am i missing something?

I think this is an old comment I forgot to remove. The current philosophy is that DepositProperties doesn't know it belongs to a bag, or at which location it is stored. That information is only known by the Deposit that holds this instance of DepositProperties. So it is correct that this save method takes the file: File as an argument.

rvanheest · 2018-08-04T08:13:28Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

@@ -336,6 +336,11 @@ class Deposit private(val baseDir: File,
    withDepositProperties(newProperties)
  }

+  /**
+   * Most methods in this library only manipulate the Bag and Deposit in memory.
+   * Saves the in-memory deposit to the file system, like calling flush() in a Stream. It writes all the bag-files and the deposit.properties.


There is no flush() on a Stream. Perhaps remove the whole like calling flush() in a Stream all together. It doesn't add much here.

Refer to the bag.save() and props.save functions (in the text and maybe also using @see.

Don't forget to fill in the @return line.

rvanheest · 2018-08-04T08:20:29Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

@@ -351,6 +356,18 @@ object Deposit {

  val depositPropertiesName = "deposit.properties"

+  /**
+   * Creates a new Deposit with an empty Bag inside.


(for all these comments, they also apply to createFromData and other documentation below)

please format the docs such that they will line out nicely

usually the @param lines don't start their documentation with a capital letter; just use lowercase letters

"if none provided SHA1 is used." --> "If none are provided, SHA-1 is used."

use backticks for the state.label, depositor.userId and bagId

use hyperlink syntax for 'nl.knaw.dans.bag.Deposit'

rvanheest · 2018-08-04T08:22:26Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

@@ -367,6 +384,21 @@ object Deposit {
      } yield new Deposit(baseDir, bag, properties)
  }

+  //TODO wouldn't it be better to be able to provide the payloadDir and a baseDir for the Deposit separately?


no, a deposit is created in the draft-area and then moved/copied to the staging area. I don't see the need for a dans-bag-lib method for that. Just use better.files.File.move/copy

What I meant to question with this is: 'Would it make sense to have an extra method that doesn't create the bag in-place, but rather at a different location, indicated by baseDir?'. However, after thinking somewhat more about this, the user of this library can just do that by itself by first copying the original data to baseDir and then calling this method.

If you agree, feel free to remove this line.

rvanheest · 2018-08-04T08:28:35Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

@@ -393,6 +431,11 @@ object Deposit {
    } yield new Deposit(baseDir, bag, properties)
  }

+  /**
+   * Returns the `baseDir` of the Deposit object
+   * @param deposit


the deposit to extract the `baseDir` from

or something along those lines

rvanheest · 2018-08-04T08:31:27Z

src/main/scala/nl/knaw/dans/bag/DepositProperties.scala

@@ -39,6 +39,12 @@ case class DepositProperties(creation: Creation = Creation(),
                             springfield: Springfield = Springfield(),
                             staged: Staged = Staged()) {

+  //TODO shouldn't this always be 'deposit.properties'? in the current Deposit.baseDir?


I think this is an old comment I forgot to remove. The current philosophy is that DepositProperties doesn't know it belongs to a bag, or at which location it is stored. That information is only known by the Deposit that holds this instance of DepositProperties. So it is correct that this save method takes the file: File as an argument.

rvanheest · 2018-08-04T08:33:17Z

src/main/scala/nl/knaw/dans/bag/DepositProperties.scala

+   * @param depositor the accountname of the depositor
+   * @param bagStore the bagId to be used for this deposit
+   * @return returns a new DepositProperties
+   */
  def empty(state: State, depositor: Depositor, bagStore: BagStore): DepositProperties = {


maybe rename this empty to from, since it is not empty at all...

rvanheest · 2018-08-04T08:33:50Z

src/main/scala/nl/knaw/dans/bag/DepositProperties.scala

+  //TODO shouldn't this always be 'deposit.properties'? in the current Deposit.baseDir?
+  /**
+   * Writes the DepositProperties to `file` on the filesystem
+   * @param file


some lines here and below are not yet filled in

jo-pol

So much confusion for me that I stopped reviewing. Am I overlooking a glossary and/or directory structure? IMO it should be possible to understand a library without specs.

jo-pol · 2018-08-06T12:27:33Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

@@ -336,6 +336,13 @@ class Deposit private(val baseDir: File,
    withDepositProperties(newProperties)
  }

+  /**
+   * Most methods in this library only manipulate the Bag and Deposit in memory.
+   * Saves the in-memory deposit to the file system. It saves all the bag files by calling `DansBag.save()`


Saves the in-memory deposit to the file system. should be the first line. The rest is detail.

I don't hope you mean you keep all payload files in memory. At first sight all the bag files seems to suggest that. Please be precise in distinguishing payload files and tag files.

jo-pol · 2018-08-06T12:45:17Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

+   * Creates a new Deposit, as a parent-directory to the `payloadDir`, moving the files in the
+   * `payloadDir` to a Bag in the Deposit.
+   *
+   * @param payloadDir the directory containing the payload files for the bag. Here the Deposit will


Just the uploaded files. i.e. the data folder in the DansV0Bag, or also files like datasetmetadata and files.xml? This is still confusing for me.

jo-pol · 2018-08-06T12:49:18Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

+   * @param bagInfo The entries to be added to `bag-info.txt`
+   * @param state The state to be set in the deposit.properties' state.label
+   * @param depositor The depositor to be set in the deposit.properties' depositor.userId
+   * @param bagStore The bagId for the bag-dir in this Deposit


Confusing for me: from a bagStore I expect something containing multiple bags hence not an id of a bag. Rather call it how you start the description: bagId.

jo-pol · 2018-08-06T12:50:37Z

src/main/scala/nl/knaw/dans/bag/Deposit.scala

+   * @param state The state to be set in the deposit.properties' state.label
+   * @param depositor The depositor to be set in the deposit.properties' depositor.userId
+   * @param bagStore The bagId for the bag-dir in this Deposit
+   * @return if successful, returns a `nl.knaw.dans.bag.Deposit` object representing the deposit located at `payloadDir`


Above you say the payloadDir is moved into the deposit, now it is the location of the deposit.

lindareijnhoudt added 2 commits August 3, 2018 10:22

Add javadoc style documentation to public methods

74a10da

Follow an intelliJ suggestion

2ee9167

lindareijnhoudt commented Aug 3, 2018

View reviewed changes

lindareijnhoudt changed the title ~~Issue #3: Add documentation to Deposit and DepositProperties~~ Issue 2 and 3: Add documentation to Deposit and DepositProperties Aug 3, 2018

rvanheest reviewed Aug 4, 2018

View reviewed changes

lindareijnhoudt added 2 commits August 6, 2018 10:31

Rename 'empty' to 'from';Complete the docs.

b79a5fd

Add param description

c265e1c

jo-pol suggested changes Aug 6, 2018

View reviewed changes

rvanheest added this to the Dans-Bag milestone Aug 6, 2018

More explicit doc on createFromData

050243d

rvanheest pushed a commit to rvanheest-DANS-KNAW/dans-bag-lib that referenced this pull request Aug 7, 2018

review of DANS-KNAW#22 and adding of extra test

bdff978

rvanheest mentioned this pull request Aug 7, 2018

review of #22 and adding of extra test #28

Merged

Richard van Heest added 2 commits August 7, 2018 14:39

review of #22 and adding of extra test

337fb7a

remove Option's from 'DepositProperties.load'

8622d80

rvanheest merged commit 73cdce6 into master Aug 7, 2018

rvanheest deleted the issue-03 branch August 7, 2018 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 2 and 3: Add documentation to Deposit and DepositProperties #22

Issue 2 and 3: Add documentation to Deposit and DepositProperties #22

lindareijnhoudt commented Aug 3, 2018 •

edited

Loading

lindareijnhoudt Aug 3, 2018

rvanheest Aug 4, 2018

lindareijnhoudt Aug 6, 2018

rvanheest Aug 6, 2018

lindareijnhoudt Aug 3, 2018

rvanheest Aug 4, 2018

rvanheest Aug 4, 2018

rvanheest Aug 4, 2018

rvanheest Aug 4, 2018

rvanheest Aug 4, 2018

rvanheest Aug 4, 2018

rvanheest Aug 4, 2018

rvanheest Aug 4, 2018

jo-pol left a comment

jo-pol Aug 6, 2018

jo-pol Aug 6, 2018

jo-pol Aug 6, 2018

jo-pol Aug 6, 2018

Issue 2 and 3: Add documentation to Deposit and DepositProperties #22

Issue 2 and 3: Add documentation to Deposit and DepositProperties #22

Conversation

lindareijnhoudt commented Aug 3, 2018 • edited Loading

Where should the reviewer @DANS-KNAW/easy start?

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

jo-pol 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

lindareijnhoudt commented Aug 3, 2018 •

edited

Loading