EXPERIMENTAL - Factor out PDF module string constants #120

carlwilson · 2016-10-03T15:03:25Z

created class for PDF MessageConstants; and
- factoring out all String constants.

codecov-io · 2016-10-03T15:11:43Z

Codecov Report

Merging #120 into integration will decrease coverage by 0.05%.
The diff coverage is 40.18%.

@@                Coverage Diff                @@
##             integration     #120      +/-   ##
=================================================
- Coverage          43.02%   42.97%   -0.06%     
  Complexity          3425     3425              
=================================================
  Files                394      398       +4     
  Lines              31679    31659      -20     
  Branches            6176     6174       -2     
=================================================
- Hits               13631    13604      -27     
- Misses             15614    15621       +7     
  Partials            2434     2434

Impacted Files	Coverage Δ	Complexity Δ
...va/edu/harvard/hul/ois/jhove/module/PdfModule.java	`70.15% <ø> (-0.15%)`	`278 <0> (ø)`
...a/edu/harvard/hul/ois/jhove/module/pdf/Stream.java	`71.01% <ø> (ø)`	`17 <0> (ø)`	⬇️
...harvard/hul/ois/jhove/module/pdf/ObjectStream.java	`71.11% <ø> (ø)`	`6 <0> (ø)`	⬇️
...rd/hul/ois/jhove/module/pdf/FileSpecification.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...rd/hul/ois/jhove/module/wave/MessageConstants.java	`0% <0%> (ø)`	`0 <0> (?)`
...arvard/hul/ois/jhove/module/pdf/StructureTree.java	`53.48% <0%> (-1.06%)`	`10 <0> (ø)`
.../edu/harvard/hul/ois/jhove/module/pdf/DocNode.java	`63.15% <0%> (-1.85%)`	`10 <0> (ø)`
...rd/hul/ois/jhove/module/aiff/MessageConstants.java	`0% <0%> (ø)`	`0 <0> (?)`
...u/harvard/hul/ois/jhove/module/pdf/PageObject.java	`53.52% <0%> (-2.48%)`	`13 <0> (ø)`
...ard/hul/ois/jhove/module/pdf/MessageConstants.java	`0% <0%> (ø)`	`0 <0> (?)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0e9222...9cef192. Read the comment docs.

- created class for PDF MessageConstants; - factoring out all String message constants to MessageConstants; and - started, but not finished, factoring out dict keys and property names. sqash

david-russo · 2016-10-06T15:00:04Z

I like how out you've factored them, but at the same time I feel they could be even further out. Like resource-file out. If we had them in such a file, it might also be useful as a source for generating the online documentation... and other language translations... the methods for working with which could all be generalised under jhove-core for the benefit of all modules... but I'm getting waaay ahead of this pull request...

Regarding which, we might want to make a note to change the message constant names for their equivalent IDs once they've been created, to avoid them possibly falling out of sync with their underlying messages.

Anyway, nice work, certainly an improvement :) I see no immediate problems with this, besides having to update the line numbers in the wiki again... Maybe leave this until after the hack day?

template based on pdf example

Needed to comment out some of the var defs, (L46, 47, 62, 76) due to poor JAVA skillz.

This will need some pruning...

pmay · 2016-10-11T15:37:19Z

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/MessageConstants.java

@@ -0,0 +1,160 @@
+/**


Missing "Invalid RoleMap" error from https://github.com/openpreserve/jhove/blob/release-1.14/jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/StructureTree.java#L249

Yvonne caught another, I'm on it ;)

…constants

- only PDF messages used in code as we have some test files; and - checked Black Box testing by breaking a message.

…nstants - bumped PDF Module version number; - finally merged old message constants for PDF module; and - tracked down some missed strings and properties in `PdfModule` class;

- string repetition in `PdfModule`; and - Eclipse artifacts into .gitignore.

david-russo · 2017-10-30T12:50:30Z

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/PdfModule.java

+
+	private static final String DICT_KEY_FONT_DESCRIPTOR = "FontDescriptor";
+	private static final String DICT_KEY_STARTXREF = "startxref";
+	private static final String DICT_KEY_BASE_FONT = base + font;


This code would be easier to read as a literal "BaseFont", cutting out the time it takes to confirm the contents of the variables, and allowing the key to be searched. They're also variables that are unlikely to need changing in the future, so I see little benefit in having them as variables. I think the same can be said for most of the variables declared on lines 116 to 121, with the exception of crop_box.

david-russo · 2017-10-30T12:53:00Z

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/PdfModule.java

+	public static final String EXT = ".pdf";
+
+	private static final String font = "Font";
+	private static final String crop_box = "CropBox";


Perhaps PROP_VAL_CROP_BOX to match the others?

david-russo · 2017-10-30T12:58:18Z

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/PdfModule.java

+	private static final String PDF_SIG_HEADER = "%" + PDF_VER1_HEADER_PREFIX;
+	private static final String POSTSCRIPT_HEADER_PREFIX = "!PS-Adobe-";
+	private static final String ENCODING_PREFIX = "ENC=";
+    private static final String NO_HEADER = "No PDF header";


Consistent space indentation to match the rest/majority of the file.

david-russo · 2017-10-30T13:10:58Z

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/Tokenizer.java

@@ -55,6 +55,9 @@
        '\u00f8','\u00f9','\u00fa','\u00fb','\u00fc','\u00fd','\u00fe','\u00ff'
    };

+    private static final String empty = "";
+    private static final String stream = "stream";


EMPTY, STREAM?

- minor changes and tweaks.

carlwilson added the in progress label Oct 3, 2016

EXPERIMENTAL - Factor out string constants

b638a56

- created class for PDF MessageConstants; - factoring out all String message constants to MessageConstants; and - started, but not finished, factoring out dict keys and property names. sqash

carlwilson force-pushed the exp-message-constants branch from 5b34c7b to b638a56 Compare October 6, 2016 09:32

carlwilson mentioned this pull request Oct 6, 2016

Documentation corrections and formatting #119

Merged

carlwilson added this to the JHOVE hack day activities milestone Oct 10, 2016

carlwilson added the feature New functionality to be developed label Oct 10, 2016

carlwilson changed the title ~~EXPERIMENTAL - Factor out string constants~~ EXPERIMENTAL - Factor out PDF module string constants Oct 10, 2016

jayGattusoNLNZ added 5 commits October 11, 2016 13:33

Create MessageConstants.java

284225e

template based on pdf example

Update MessageConstants.java

7fba761

Update MessageConstants.java

9dcb6a1

Create MessageConstants.java

55bde67

Needed to comment out some of the var defs, (L46, 47, 62, 76) due to poor JAVA skillz.

Create MessageConstants.java

a6bdea2

This will need some pruning...

pmay reviewed Oct 11, 2016

View reviewed changes

carlwilson and others added 7 commits October 11, 2016 17:38

Merge branch 'integration' into exp-message-constants

e44dd9b

Merge remote-tracking branch 'upstream/integration' into exp-message-…

9058f6a

…constants

FIX - Compile errors in message constants

1b35cb9

- only PDF messages used in code as we have some test files; and - checked Black Box testing by breaking a message.

Merge branch 'integration' into exp-message-constants

f69e141

Merge branch 'integration' into exp-message-constants

f7c31a5

Merge remote-tracking branch 'origin/integration' into exp-message-co…

4dbe1bb

…nstants - bumped PDF Module version number; - finally merged old message constants for PDF module; and - tracked down some missed strings and properties in `PdfModule` class;

FIX - Codacy warnings

0ae6e45

- string repetition in `PdfModule`; and - Eclipse artifacts into .gitignore.

david-russo reviewed Oct 30, 2017

View reviewed changes

REV - Post review from @david-russo

9cef192

- minor changes and tweaks.

carlwilson merged commit fc4ed1b into integration Oct 30, 2017

carlwilson removed the in progress label Oct 30, 2017

carlwilson deleted the exp-message-constants branch October 30, 2017 18:51

rgfeldman added a commit to rgfeldman/jhove that referenced this pull request Apr 10, 2019

issue openpreserve#120

3f5b47b

rgfeldman added a commit to rgfeldman/jhove that referenced this pull request Apr 10, 2019

openpreserve#119, openpreserve#120, openpreserve#121, openpreserve#122

6fddf60

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EXPERIMENTAL - Factor out PDF module string constants #120

EXPERIMENTAL - Factor out PDF module string constants #120

carlwilson commented Oct 3, 2016 •

edited

Loading

codecov-io commented Oct 3, 2016 •

edited by codecov bot

Loading

david-russo commented Oct 6, 2016 •

edited

Loading

pmay Oct 11, 2016

carlwilson Oct 11, 2016

david-russo Oct 30, 2017

david-russo Oct 30, 2017

david-russo Oct 30, 2017

david-russo Oct 30, 2017

EXPERIMENTAL - Factor out PDF module string constants #120

EXPERIMENTAL - Factor out PDF module string constants #120

Conversation

carlwilson commented Oct 3, 2016 • edited Loading

codecov-io commented Oct 3, 2016 • edited by codecov bot Loading

Codecov Report

david-russo commented Oct 6, 2016 • edited Loading

pmay Oct 11, 2016

Choose a reason for hiding this comment

carlwilson Oct 11, 2016

Choose a reason for hiding this comment

david-russo Oct 30, 2017

Choose a reason for hiding this comment

david-russo Oct 30, 2017

Choose a reason for hiding this comment

david-russo Oct 30, 2017

Choose a reason for hiding this comment

david-russo Oct 30, 2017

Choose a reason for hiding this comment

carlwilson commented Oct 3, 2016 •

edited

Loading

codecov-io commented Oct 3, 2016 •

edited by codecov bot

Loading

david-russo commented Oct 6, 2016 •

edited

Loading