Add support for getting value as Period by nrktkt · Pull Request #478 · lightbend/config

nrktkt · 2017-07-02T20:04:45Z

from issue #473

I think parseDuration and parsePeriod could probably be re-worked to be a little more dry (something with ChronoUnit.isTimeBased rather than TimeUnit.toNanos), but didn't feel the need to do that now as this is a pretty superficial feature.

from issue lightbend#473

nrktkt · 2017-07-02T20:26:06Z

Signed cla

havocp · 2017-07-04T19:50:30Z

Thanks! Looking pretty good. We might consider a unit test in UnitParserTest.scala covering some of the weird corner cases (especially error handling), and maybe update HOCON.md to describe how periods are parsed differently from durations? there's a section in there for durations already.

nrktkt · 2017-07-22T22:05:38Z

@havocp how's that looking?

havocp

I think this is mergeable now; any thoughts from anyone else? Thanks so much!

havocp · 2017-07-22T22:40:41Z

config/src/main/java/com/typesafe/config/impl/SimpleConfig.java

+        return parsePeriod((String) v.unwrapped(), v.origin(), path);
+    }
+
+    public TemporalAmount getTemporal(String path){


does this one need @OverRide?

yes, let me add that

havocp · 2017-07-22T22:44:44Z

config/src/main/java/com/typesafe/config/Config.java

+    /**
+     * Gets a value as a java.time.temporal.TemporalAmount.
+     * This method will first try get get the value as a java.time.Duration, and if unsuccessful,
+     * then as a java.time.Period.


I feel like the ambiguous values here ("m" and no units?) are fairly dangerous perhaps ... an option would be to outright prohibit them... though that could be confusing also. I'm OK with what you have here but do wonder if there's an option that doesn't leave a pitfall for whoever is writing the config.

I had that thought as well. I think it it can be confusing, but most applications will use either getPeriod or getDuration and will not need to worry. For me it's just balancing how many people will be confused by m being unavailable vs how many will use getTemporal.

TemporalAmount is not really meant to be used in application code. According to the javadoc:

This interface is a framework-level interface that should not be widely used in application code. Instead, applications should create and pass around instances of concrete types, such as Period and Duration.

If you're going to force users to decide how to handle the TemporalAmount returned from this method, you might as well let them implement the relatively simple logic in getTemporal themselves, so they can decide how to handle the ambiguity of minutes/months.

In a practical sense, I think it would be more useful to have getDuration support weeks, months, and years as units using standard lengths for those units. In most cases the underlying API I'm using (e.g. max-age for a cookie) expects a precise number of nanoseconds, so a Period is usually not what I want anyway.

I had assumed that the config library would be used by framework creators as well as directly by applications. Is there a convenient way we can surface the ability to get a TemporalAmount without putting it directly on the Config interface?

@kag0 can you describe a use case for a framework creator?

Is there a convenient way we can surface the ability to get a TemporalAmount without putting it directly on the Config interface?

What does it mean to "get a TemporalAmount" to you? What kind of temporal amounts should we support?

In this case "get a TemporalAmount" to me just means getting either a Duration or Period but not knowing which.

I could imagine a framework creator might have some setting where they might want to do OffsetDateTime.now().plus(maintenanceReminderInterval) and wouldn't want to constrain the user to a period or duration, but do want to prevent taking a TemporalAmount directly.

Yeah, I feel like in that case you're adding the possibility to return Duration as a hack because Period doesn't support units smaller than days. Ideally you want to be able to return a type that supports both, such as PeriodDuration from threeten-extra.

gmethvin

I would also expect to be able to write

foo = P1Y2M3D

using the ISO-8601 period format.

It seems like the current implementation does not support multiple temporal units, so that format would be an unambiguous way to provide support for all possible Periods.

gmethvin · 2017-07-23T08:20:57Z

config/src/main/java/com/typesafe/config/Config.java

+    /**
+     * Gets a value as a java.time.temporal.TemporalAmount.
+     * This method will first try get get the value as a java.time.Duration, and if unsuccessful,
+     * then as a java.time.Period.


TemporalAmount is not really meant to be used in application code. According to the javadoc:

This interface is a framework-level interface that should not be widely used in application code. Instead, applications should create and pass around instances of concrete types, such as Period and Duration.

If you're going to force users to decide how to handle the TemporalAmount returned from this method, you might as well let them implement the relatively simple logic in getTemporal themselves, so they can decide how to handle the ambiguity of minutes/months.

In a practical sense, I think it would be more useful to have getDuration support weeks, months, and years as units using standard lengths for those units. In most cases the underlying API I'm using (e.g. max-age for a cookie) expects a precise number of nanoseconds, so a Period is usually not what I want anyway.

nrktkt · 2017-07-24T01:32:13Z

I would also expect to be able to write
foo = P1Y2M3D
using the ISO-8601 period format.

Is there support currently for doing this with durations? If so then I think there is an argument for adding it. But if not, can we make it a separate issue? Presumably users would use the smallest applicable unit to describe the period they needed.

gmethvin · 2017-07-24T22:46:15Z

Is there support currently for doing this with durations?

No, but that's not a problem because Durations are not based on the ISO-8601 calendar. Duration assumes a standard length for each of the time units involved. That's not the case for periods. The period "1 month and 15 days" could correspond to "45 days", "46 days", etc. depending on when on the calendar it starts. So for periods we need to store the individual fields.

That said, Duration.parse allows you to parse a format like "PT1H2M34S".

If the goal is simply to support a number with unit it would be far easier to use, and more semantically correct, if we returned a class representing a pair of Long and ChronoUnit. Period is not the proper type to use.

nrktkt · 2017-07-24T23:18:47Z

The period "1 month and 15 days" could correspond to "45 days", "46 days", etc.

A very good point. With that in mind, parsing ISO periods should definitely be supported.
P1Y2M3D
1 month
2 days
1y
Should all be valid values.

gmethvin · 2017-07-25T08:01:39Z

Expanding on my earlier proposal, we could have a general method to parse a numeric value with a chronological unit. We could have a type like this:

public final class ConfigChronoAmount {
  private final long amount; // maybe should be BigDecimal/double?
  private final ChronoUnit unit;
  // ... constructor, getters ...
}

and add a method ConfigChronoAmount getChrono(String path) that does similar parsing as parseDuration (perhaps consolidate the methods).

Note that it is trivial to convert this to a duration using the estimated values of the units (unit.getDuration().multipliedBy(amount)). I would argue that's the most common situation. If I set my timeout to 1 year it's fine to use a value of 1 year = 365 days instead of a different one depending on the calendar. The year is just a shorthand.

The only thing I'm not sure about is whether we should support fractional units like 1.5 months. It would be nice to do that from a user standpoint, but would be more complicated to convert to a Duration cleanly (probably the easiest way is BigDecimal math).

Perhaps all that is outside the intended goal of this PR. Just mentioning the getChrono idea since I think that would solve the problem of allowing users and framework developers to manually handle the units the way they want.

2m

Looking great! Merging this. For those who are interested in support for more formats, please open a separate issue.

marconilanna · 2017-10-06T23:29:54Z

config/src/main/java/com/typesafe/config/Config.java

+     * "10d" or "5w" as documented in the <a
+     * href="https://github.com/typesafehub/config/blob/master/HOCON.md">the
+     * spec</a>. This method never returns null.
+     *


This should be @since 1.3.2

marconilanna · 2017-10-06T23:31:22Z

config/src/main/java/com/typesafe/config/Config.java

+     * Gets a value as a java.time.temporal.TemporalAmount.
+     * This method will first try get get the value as a java.time.Duration, and if unsuccessful,
+     * then as a java.time.Period.
+     * This means that values like "5m" will be parsed as 5 minutes rather than 5 months


Missing the @since 1.3.2 annotation

Add support for getting value as Period

Add support for getting value as Period

ac20019

from issue lightbend#473

nrktkt force-pushed the parse-period branch from 974ae1d to ac20019 Compare July 2, 2017 20:10

update HOCON.md and add test to UnitParserTest

c5e77d5

havocp approved these changes Jul 22, 2017

View reviewed changes

override

69423ab

gmethvin reviewed Jul 23, 2017

View reviewed changes

2m approved these changes Oct 6, 2017

View reviewed changes

2m merged commit aa69584 into lightbend:master Oct 6, 2017

2m mentioned this pull request Oct 6, 2017

Parse TemporalAmounts besides Duration #473

Closed

marconilanna reviewed Oct 6, 2017

View reviewed changes

marconilanna mentioned this pull request Oct 6, 2017

Missing methods: getPeriodList and getTemporalList #503

Closed

ches mentioned this pull request Feb 16, 2018

Support the HOCON 1.3.2 Period Format for java.time.Period pureconfig/pureconfig#359

Closed

aalleexxeeii pushed a commit to aalleexxeeii/typesafe-config that referenced this pull request Jun 6, 2018

Merge pull request lightbend#478 from kag0/parse-period

4036864

Add support for getting value as Period

Conversation

nrktkt commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrktkt commented Jul 2, 2017

Uh oh!

havocp commented Jul 4, 2017

Uh oh!

nrktkt commented Jul 22, 2017

Uh oh!

havocp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmethvin Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmethvin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrktkt commented Jul 24, 2017

Uh oh!

gmethvin commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrktkt commented Jul 24, 2017

Uh oh!

gmethvin commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nrktkt commented Jul 2, 2017 •

edited

Loading

gmethvin Jul 24, 2017 •

edited

Loading

gmethvin left a comment •

edited

Loading

gmethvin commented Jul 24, 2017 •

edited

Loading

gmethvin commented Jul 25, 2017 •

edited

Loading