Deprecate publicly-visible properties on Style in path.

Deprecate publicly-visible properties on Style in path.

R=rnystrom@google.com

Committed: https://code.google.com/p/dart/source/detail?r=36893

[kevmoo, 2014-05-29 01:52:35]

On 2014/05/29 00:51:35, nweiz wrote:

DBQ: why? Just curious.

[Bob Nystrom, 2014-05-29 15:45:39]

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/context.dart
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:38: "allowed.");
This is a breaking change, I think.

One option would to have the path package sniff out whether the Style passed in
is an InternalStyle or not. If it is, we can use the new faster parsing logic.
Otherwise, we can fall back to the old RegExp behavior.

Then, in 2.0.0, we remove support for the old behavior.

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/parsed_path...
File pkg/path/lib/src/parsed_path.dart (right):

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/parsed_path...
pkg/path/lib/src/parsed_path.dart:10: // TODO(rnystrom): Make this public?
Can remove this TODO.

[nweiz, 2014-05-29 20:17:00]

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/context.dart
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:38: "allowed.");
On 2014/05/29 15:45:39, Bob Nystrom wrote:
> This is a breaking change, I think.
> 
> One option would to have the path package sniff out whether the Style passed
in
> is an InternalStyle or not. If it is, we can use the new faster parsing logic.
> Otherwise, we can fall back to the old RegExp behavior.
> 
> Then, in 2.0.0, we remove support for the old behavior.

This could technically break someone, but as you mentioned, I don't think any
user-defined styles exist in the wild. There's some leeway in versioning
packages for theoretically-but-not-practically breaking changes like adding a
top-level definition; I don't think it's worth bifurcating the Context and
ParsedPath classes to support a use case that no one will ever care about.

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/parsed_path...
File pkg/path/lib/src/parsed_path.dart (right):

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/parsed_path...
pkg/path/lib/src/parsed_path.dart:10: // TODO(rnystrom): Make this public?
On 2014/05/29 15:45:39, Bob Nystrom wrote:
> Can remove this TODO.

Done.

[Bob Nystrom, 2014-05-29 23:54:10]

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/context.dart
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:38: "allowed.");
On 2014/05/29 20:17:00, nweiz wrote:
> On 2014/05/29 15:45:39, Bob Nystrom wrote:
> > This is a breaking change, I think.
> > 
> > One option would to have the path package sniff out whether the Style passed
> in
> > is an InternalStyle or not. If it is, we can use the new faster parsing
logic.
> > Otherwise, we can fall back to the old RegExp behavior.
> > 
> > Then, in 2.0.0, we remove support for the old behavior.
> 
> This could technically break someone, but as you mentioned, I don't think any
> user-defined styles exist in the wild. There's some leeway in versioning
> packages for theoretically-but-not-practically breaking changes like adding a
> top-level definition; I don't think it's worth bifurcating the Context and
> ParsedPath classes to support a use case that no one will ever care about.

If we don't think this will break anyone, then we aren't supporting user-defined
styles at all.

Given that, why have a deprecation period at all? Why not just yank the stuff
out now?

[nweiz, 2014-05-30 00:52:14]

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/context.dart
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/304843002/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:38: "allowed.");
On 2014/05/29 23:54:10, Bob Nystrom wrote:
> On 2014/05/29 20:17:00, nweiz wrote:
> > On 2014/05/29 15:45:39, Bob Nystrom wrote:
> > > This is a breaking change, I think.
> > > 
> > > One option would to have the path package sniff out whether the Style
passed
> > in
> > > is an InternalStyle or not. If it is, we can use the new faster parsing
> logic.
> > > Otherwise, we can fall back to the old RegExp behavior.
> > > 
> > > Then, in 2.0.0, we remove support for the old behavior.
> > 
> > This could technically break someone, but as you mentioned, I don't think
any
> > user-defined styles exist in the wild. There's some leeway in versioning
> > packages for theoretically-but-not-practically breaking changes like adding
a
> > top-level definition; I don't think it's worth bifurcating the Context and
> > ParsedPath classes to support a use case that no one will ever care about.
> 
> If we don't think this will break anyone, then we aren't supporting
user-defined
> styles at all.
> 
> Given that, why have a deprecation period at all? Why not just yank the stuff
> out now?

Because it's (somewhat) plausible that people are calling
`path.platform.style.rootPattern` et al.

[Bob Nystrom, 2014-05-30 16:36:25]

LGTM.

[nweiz, 2014-06-02 19:06:28]

Committed patchset #2 manually as r36893 (presubmit successful).