[SIL.Lift] Clean up LiftParser #1420
Conversation
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @imnasnainaec)
-- commits line 9 at r1:
ReadField was added, not "fixed"
SIL.Lift/Parsing/LiftParser.cs line 388 at r1 (raw file):
var extensible = new Extensible { // Actually not part of extensible (as of 8/1/2007).
Do we know what this comment is referring to or why it is significant? Is it still not part of extensible? If not, after 18 years, I'd guess it never will be!
SIL.Lift/Parsing/LiftParser.cs line 392 at r1 (raw file):
}; //todo: figure out how to actually look it up:
Our usual standard is to put this in all caps, like this:
// TODO: ...
SIL.Lift/Parsing/LiftParser.cs line 444 at r1 (raw file):
/// <summary> /// careful, can't return null, so give MinValue
This is not a "summary", nor does it accurately reflect what the code does. In cases like this, we often just have a summary with no element, but it should say what the method does. Since the defaultDateTime param already describes what we do if the attribute is not found, this particular comment would be unnecessary even if it were accurate.
SIL.Lift/Parsing/LiftParser.cs line 617 at r1 (raw file):
if (Validator.GetLiftVersion(pathToLift) != Validator.LiftVersion) throw new LiftFormatException( "Programmer should migrate the lift file before calling this method.");
I think Lift should be capitalized to be consistent with other messages. (Actually, LIFT is an acronym -- see the above summary -- but maybe someone decided it should be capitalized as a proper noun instead.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @tombogle)
Previously, tombogle (Tom Bogle) wrote…
ReadField was added, not "fixed"
This will be lost in the squash. (Private method ReadField was added in a previous commit and fixed in this commit.)
SIL.Lift/Parsing/LiftParser.cs line 388 at r1 (raw file):
Previously, tombogle (Tom Bogle) wrote…
Do we know what this comment is referring to or why it is significant? Is it still not part of extensible? If not, after 18 years, I'd guess it never will be!
Done.
SIL.Lift/Parsing/LiftParser.cs line 392 at r1 (raw file):
Previously, tombogle (Tom Bogle) wrote…
Our usual standard is to put this in all caps, like this:
// TODO: ...
Done.
SIL.Lift/Parsing/LiftParser.cs line 444 at r1 (raw file):
Previously, tombogle (Tom Bogle) wrote…
This is not a "summary", nor does it accurately reflect what the code does. In cases like this, we often just have a summary with no element, but it should say what the method does. Since the defaultDateTime param already describes what we do if the attribute is not found, this particular comment would be unnecessary even if it were accurate.
Done.
SIL.Lift/Parsing/LiftParser.cs line 617 at r1 (raw file):
Previously, tombogle (Tom Bogle) wrote…
I think Lift should be capitalized to be consistent with other messages. (Actually, LIFT is an acronym -- see the above summary -- but maybe someone decided it should be capitalized as a proper noun instead.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)
SIL.Lift/Parsing/LiftParser.cs line 624 at r2 (raw file):
if (ChangeDetector != null && ChangeDetector.CanProvideChangeRecord) { ProgressMessage = "Detecting changes to the LIFT file...";
Let's not do this as part of this PR, but if these strings are displayed to the user, we should consider making them localizable.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @tombogle)
SIL.Lift/Parsing/LiftParser.cs line 624 at r2 (raw file):
Previously, tombogle (Tom Bogle) wrote…
Let's not do this as part of this PR, but if these strings are displayed to the user, we should consider making them localizable.
Ah, I guess somebody's script could be relying on particular progress messages. Reverted.
I don't see anything outside SIL.Windows.Forms* or TestApps/ with using L10NSharp;, so I'm not inclined to start localization in SIL.Lift just for this file.
|
Previously, imnasnainaec (D. Ror.) wrote…
Right. This would require some analysis. I'm not sure how these progress messages are used. There are strategies for localization in contexts where we can't directly depend on L10nSharp, but first we'd need to see if it actually makes sense. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)
Cleanup started during #1414, but no changes to this file were necessary for that pr.
This change is