phet commented on code in PR #3535:
URL: https://github.com/apache/gobblin/pull/3535#discussion_r940753420
##########
gobblin-data-management/src/main/java/org/apache/gobblin/data/management/copy/TimeAwareRecursiveCopyableDataset.java:
##########
@@ -68,7 +68,7 @@ public TimeAwareRecursiveCopyableDataset(FileSystem fs, Path
rootPath, Propertie
this.datePattern = properties.getProperty(DATE_PATTERN_KEY);
this.currentTime = properties.containsKey(DATE_PATTERN_TIMEZONE_KEY) ?
LocalDateTime.now(
- DateTimeZone.forID(DATE_PATTERN_TIMEZONE_KEY))
+ DateTimeZone.forID(properties.getProperty(DATE_PATTERN_TIMEZONE_KEY)))
: LocalDateTime.now(DateTimeZone.forID(DEFAULT_DATE_PATTERN_TIMEZONE));
Review Comment:
that's a worthwhile pursuit.... but complicated because our installed/legacy
user base may have come to depend on the old names. in general they can't be
removed... only given an alias. this seems in the spirit of attacking that
proliferation:
https://github.com/apache/gobblin/blob/d9ae5353c74fdcd385835fca9b586b3fdb90971b/gobblin-utility/src/main/java/org/apache/gobblin/util/deprecation/DeprecationUtils.java#L43
but, yes, I agree on the benefit of aligning under a common name for the
'same' concept in use across multiple classes. at this point, I'm don't have
an opinion on which name deserves to 'win' and be enshrined as the 'standard
property name'.
(all that... and... my original suggestion was just to DRY out the code,
eliminating the duplicated calls, not specifically to standardize on a common
property name.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
