Remove NodaCultureInfo, and make NodaFormatInfo internal #131

Closed
GoogleCodeExporter opened this Issue Mar 15, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@GoogleCodeExporter
NodaCultureInfo and NodaFormatInfo have a confusing design:
- they should probably either be a single object or have a one-way 
relationship, not the bidirectional relationship they currently have.
- they are optionally-mutable (like CultureInfo, to be fair)
- NodaCultureInfo creates and caches a NodaFormatInfo on demand, but doesn't 
play nicely with mutability (see issue 130).

Offhand, it's not entirely clear what we'd lose if we made both classes 
internal for 1.0, other than the ability to define a custom default format for 
Noda Time-specific objects, or specify a custom format for compound formatters. 
 (Note that you can always format explicitly via a pattern.)

I'd like to explore what impact making the two classes internal would have for 
1.0.  We can always reverse that decision for 1.1, if we decide it was a bad 
idea, but doing so would allow us to spend a bit more time making sense of this 
without committing to it before a 2.0 release (see issue 96).

Original issue reported on code.google.com by malcolm.rowe on 26 Oct 2012 at 8:35

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

The effort required would appear to be:

- make the classes internal.
- make the FormatInfo property in the pattern classes internal (or possibly 
remove it)
- make the public  Create methods in the pattern classes take an 
IFormatProvider instead, and get the NodaFormatInfo via 
NodaFormatInfo.GetInstance(provider).
- ditto for WithFormatInfo(), which probably should be renamed.
- remove mention of NodaFormatInfo from the documentation.

I note also that it would be almost trivial to remove NodaCultureInfo entirely 
at that point: it only has a reference from a NodaFormatInfo, and I suspect 
that could be the CultureInfo passed to the constructor instead (which then 
avoids the crazy reflexive referencing that's updated in Clone()).  

Original comment by malcolm.rowe on 26 Oct 2012 at 7:46

The effort required would appear to be:

- make the classes internal.
- make the FormatInfo property in the pattern classes internal (or possibly 
remove it)
- make the public  Create methods in the pattern classes take an 
IFormatProvider instead, and get the NodaFormatInfo via 
NodaFormatInfo.GetInstance(provider).
- ditto for WithFormatInfo(), which probably should be renamed.
- remove mention of NodaFormatInfo from the documentation.

I note also that it would be almost trivial to remove NodaCultureInfo entirely 
at that point: it only has a reference from a NodaFormatInfo, and I suspect 
that could be the CultureInfo passed to the constructor instead (which then 
avoids the crazy reflexive referencing that's updated in Clone()).  

Original comment by malcolm.rowe on 26 Oct 2012 at 7:46

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

We're going to try to do the above.  This will also obsolete issue 133 and 
issue 130.

Original comment by malcolm.rowe on 27 Oct 2012 at 8:54

We're going to try to do the above.  This will also obsolete issue 133 and 
issue 130.

Original comment by malcolm.rowe on 27 Oct 2012 at 8:54

@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

I decided to make the Create methods of the pattern classes take a concrete 
CultureInfo to avoid a problem similar to issue 134.  (I'm assuming that 
switching to the interface later would be a binary-compatible change; if not, 
we could presumably introduce an overload.)

Original comment by malcolm.rowe on 27 Oct 2012 at 9:46

  • Changed title: Remove NodaCultureInfo, and make NodaFormatInfo internal
  • Changed state: Started
I decided to make the Create methods of the pattern classes take a concrete 
CultureInfo to avoid a problem similar to issue 134.  (I'm assuming that 
switching to the interface later would be a binary-compatible change; if not, 
we could presumably introduce an overload.)

Original comment by malcolm.rowe on 27 Oct 2012 at 9:46

  • Changed title: Remove NodaCultureInfo, and make NodaFormatInfo internal
  • Changed state: Started
@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

This issue was closed by revision 135e55ca4331.

Original comment by malcolm.rowe on 27 Oct 2012 at 11:55

  • Changed state: Fixed
This issue was closed by revision 135e55ca4331.

Original comment by malcolm.rowe on 27 Oct 2012 at 11:55

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

Show comment
Hide comment
@GoogleCodeExporter

GoogleCodeExporter Mar 15, 2015

Original comment by malcolm.rowe on 10 Nov 2012 at 10:20

  • Added labels: Milestone-1.0.0
  • Removed labels: Milestone-1.0

Original comment by malcolm.rowe on 10 Nov 2012 at 10:20

  • Added labels: Milestone-1.0.0
  • Removed labels: Milestone-1.0

@malcolmr malcolmr added the bug label Mar 15, 2015

@malcolmr malcolmr modified the milestone: 1.0.0 Mar 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment