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

Adapt parsing Bagit Profile due to specification. #128

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

Conversation

VolkerHartmann
Copy link

@VolkerHartmann VolkerHartmann commented Nov 12, 2018

Adapt parsing Bagit Profile due to specification. (https://github.com/bagit-profiles/bagit-profiles)

Inclusion of "Contact-Name," "Contact-Phone" and "Contact-Email," as defined in the BagIt spec, is not required but is encouraged.
-> Add "Contact-Phone"
-> "Contact-Name" and "Contact-Email" are now optional
Add test for minimal profile
Adapt other tests.
Bag-Info:
The parameters "required" is 'false' and "repeatable" is 'true' by default.
Changed implementation accordingly.
("repeatable": Not used yet inside the library!?)

Please ensure you have completed the following before submitting:

  • Ran all tests to ensure existing functionality wasn't broken
  • Ran all quality assurance checks and fixed any new errors or warnings, which include:

Note: you can complete both boxes by running and fixing warnings/errors with gradle clean check

  • Code is self documenting or a short comment when self documenting isn't possible

Volker Hartmann added 2 commits November 12, 2018 10:32
…/bagit-profiles/bagit-profiles)

Inclusion of "Contact-Name," "Contact-Phone" and "Contact-Email," as defined in the BagIt spec, is not required but is encouraged.
-> Add "Contact-Phone"
-> "Contact-Name" and "Contact-Email" are now optional
Add test for minimal profile
Adapt other tests.
Bag-Info:
The parameters "required" is 'false' and "repeatable" is 'true' by default.
Changed implementation accordingly.
("repeatable": Not used yet inside the library!?)
@coveralls
Copy link

coveralls commented Nov 12, 2018

Coverage Status

Coverage increased (+0.04%) to 98.319% when pulling e6ce9a7 on VolkerHartmann:master into 2b7002e on LibraryOfCongress:master.



// Read required tags first
// due to specification defined at https://github.com/bagit-profiles/bagit-profiles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure what this comment means. Why do you need to read required tags first?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First the required tags are read and then the optional tags because they have to be handled differently. It's just to keep the code clearer.
If one of the required ones do not already exist, the optional ones are no longer read at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless there is an error being thrown that I don't know about, how are you stopping if a required tag is missing? To put it another way, what would break if you moved those if not null statements to being read first?

Perhaps a better way to make it cleaner would be to make another method that contained all the reads for optional tags. Something like

    final JsonNode contactNameNode = bagitProfileInfoNode.get("Contact-Name");
    if (contactNameNode != null) {
      final String contactName = contactNameNode.asText();
      logger.debug(messages.getString("contact_name"), contactName);
      profile.setContactName(contactName);
    }
     final JsonNode contactEmailNode = bagitProfileInfoNode.get("Contact-Email");
    if (contactEmailNode != null) {
      final String contactEmail = contactEmailNode.asText();
      logger.debug(messages.getString("contact_email"), contactEmail);
      profile.setContactEmail(contactEmail);
    }
     final JsonNode contactPhoneNode = bagitProfileInfoNode.get("Contact-Phone");
    if (contactPhoneNode != null) {
      final String contactPhone = contactPhoneNode.asText();
      logger.debug(messages.getString("contact_phone"), contactPhone);
      profile.setContactPhone(contactPhone);
    }
}

Copy link
Author

@VolkerHartmann VolkerHartmann Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one of the required tags is not present, a NullPointerException is thrown.

final String profileIdentifier = bagitProfileInfoNode.get("BagIt-Profile-Identifier").asText();

See documentation of class BagLinter:

This class is only to be used on VALID bags, using it on un-validated bags may result in
exceptions being thrown (like {@link java.io.IOException} )

Therefore, no error handling is done anywhere in the code.

@jscancella
Copy link
Contributor

Awesome work! I had a couple questions but overall really great work

@VolkerHartmann
Copy link
Author

@jscancella: Have you seen my changes yet? It would be great if you'd take a look at them.

@jscancella
Copy link
Contributor

Looks good to me. Now you just need someone at LC to merge it

@VolkerHartmann
Copy link
Author

@acdha It'd be great if you could merge the code.

@acdha
Copy link
Member

acdha commented Nov 27, 2018

@VolkerHartmann I'm not a member of this particular repository

@kba
Copy link

kba commented Nov 27, 2018

Who is currently maintaining bagit-java at @LibraryOfCongress?

#125 and #128 seem ready for merging but the discussion sort of fizzled out.

Is there some way to help out with testing/deployment etc.?

@dbrunton

@jscancella
Copy link
Contributor

@kba I suspect after I left the answer is no one.

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.

5 participants