Skip to content
This repository was archived by the owner on Oct 18, 2024. It is now read-only.

add lowercase versions of the constants #46

Closed
wants to merge 1 commit into from
Closed

Conversation

devoncarew
Copy link
Contributor

@pq @kevmoo

@kevmoo
Copy link
Contributor

kevmoo commented Oct 27, 2018

CC @natebosch @jakemac53 @grouma

Thoughts on this?

I hate adding the other set of constants without deprecating the old ones.

I also cringe at all of the deprecation warnings it'll cause...hrm...

@pq
Copy link
Contributor

pq commented Oct 27, 2018

My sense is we should bite the bullet and deprecate. Updating references will be a bummer but a one-time thing. I'm happy to pitch in some of the community packages too.

@@ -271,48 +271,61 @@ class Level implements Comparable<Level> {
const Level(this.name, this.value);

/// Special key to turn on logging for all levels ([value] = 0).
static const Level ALL = const Level('ALL', 0);
static const Level all = const Level('ALL', 0);
static const Level ALL = all;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest we go ahead and @deprecate all these.

];

static const List<Level> LEVELS = levels;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And deprecate this too. 👍

@jakemac53
Copy link
Contributor

jakemac53 commented Oct 29, 2018

My feeling is this provides to little value to justify the churn it causes. This is forcing real work on lots of people for what is the equivalent of a style nitpick.

@devoncarew
Copy link
Contributor Author

I hate adding the other set of constants without deprecating the old ones.

The all caps constants look old; definitely not a P1 to rename them though. I didn't deprecate the old ones just because of the cost downstream.

I also cringe at all of the deprecation warnings it'll cause...hrm...

Deprecating them would cause a lot of downstream work - likely more work than we'd actually want to do.

@jakemac53
Copy link
Contributor

The all caps constants look old; definitely not a P1 to rename them though. I didn't deprecate the old ones just because of the cost downstream.

It is confusing to have both the lowercase and uppercase versions though. This does remove my concerns around downstream work, but it provides less value (arguably negative value because now there are 2 ways to do exactly the same thing).

@kevmoo
Copy link
Contributor

kevmoo commented Oct 29, 2018

arguably negative value because now there are 2 ways to do exactly the same thing

AGREED!

@devoncarew
Copy link
Contributor Author

devoncarew commented Oct 30, 2018

I'll close this, but we may want to consider a pre-deprecation stage in cases like this. It'd would be useful to be able to switch clients to the new thing, w/o deprecation messages existing yet on the old one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Level constants screaming CAPS
5 participants