sdk icon indicating copy to clipboard operation
sdk copied to clipboard

log is defined in math and developer packages and has a completely different purpose, we should avoid name collision in the standard library when it make sense

Open stephane-archer opened this issue 1 year ago • 11 comments

Change Intent

Use case

log is defined in math and developer packages and has a completely different purpose. not the best development experience. let's say you have:

import developer;

foo() {
int i = 0;
log(i)
}

then you need the math package for bar, you see the collision and do change your import too import developer as developer; import math then you add developer. when the compiler is complaining you add up with the following code

import developer as developer;

foo() {
int i = 0;
log(i) // BOOM!
}

bar() {
// my math function
}

Proposal

I think the standard library should avoid collision as much as possible. devLog could be an acceptable name the math library and the need to log thing is quite commun

Justification

Developper experience

stephane-archer avatar Apr 29 '24 11:04 stephane-archer

remember to always alias the math library is not the best developer experience.

stephane-archer avatar Apr 29 '24 11:04 stephane-archer

Please don't use breaking-change-request template if you are just making a feature request or filing a bug. This template is used for team specific process.

mraleph avatar Apr 29 '24 11:04 mraleph

I think the recommended way to handle this is to use some combination of show, hide and as to avoid the clash.

I think changing the name of dart:developer log to something else is not worth the trouble at this point.

cc @lrhn for final judgement.

mraleph avatar Apr 29 '24 11:04 mraleph

@mraleph From my understanding, changing a function name is a breaking change because it changes the public API of the package but I'm maybe missing something. What I propose more globally is to reduce the need for show, hide, and as when using the standard library. I see these features as something to avoid conflicting names but not something you use everywhere in your project

stephane-archer avatar Apr 29 '24 13:04 stephane-archer

@stephane-archer it is indeed a breaking change (and the one we are rather unlikely to decide to do), but that does not mean you have to file a breaking change request - breaking change requests are used when we are executing the process of rolling out actual breaking changes and are usually filed by the person who owns the change.

mraleph avatar Apr 29 '24 13:04 mraleph

Would have been nice to avoid the name clash, if we had been aware of it from the start.

The best change would have been to change the log in dart:math to ln, or maybe to loge (so we could have log2 and log10 as well).

Changing anything today is very unlikely to be worth the effort. Renaming log to ln would likely mean keeping the alias log around for a long time (until Dart 4.0 or so), which means that it doesn't actually do anything in the short term.

The problem is limited, and has an easy workaround. If the same library uses both dart:developer and dart:math, and uses the log from both (if it only used one, it could just hide the log from the other), then it's annoying. But nothing more, the author will have to import one of the libraries with a prefix (instead or as well, with a hide on the non-prefixed one). This is standard name clash handling, something a Dart author is expected to be able to do, and name clashes can just as easily happen with third party libraries, so this isn't something special to know for dart: libraries. The workaround is always the same.

So, no current plan to rename anything. Unless it becomes a much larger problem, I don't see it being worth even the beginning effort of renaming log to ln in dart:math, and adding:

@Deprecated("Use ln instead")
double log(num value) => ln(value);

Even if it's better, it's still a huge impact on everybody who has ever used the name log.

If we want to do this, we should also create a fix that rewrites log to ln. Then at least the impact on users will be limited to running dart fix when they get deprecation warnings, and eventually, maybe, we can remove log in Dart 4.0 or 5.0.

lrhn avatar Apr 29 '24 13:04 lrhn

@Irhn I would be in favor of:

@Deprecated("Use ln instead")
double log(num value) => ln(value);

To me, this is a progressive change, that can easily be fixed with dart fix and that would make things better in the long run. but I understand if you consider the cost of change to be too high for existing users.

stephane-archer avatar Apr 29 '24 14:04 stephane-archer

@stephane-archer it is indeed a breaking change (and the one we are rather unlikely to decide to do), but that does not mean you have to file a breaking change request - breaking change requests are used when we are executing the process of rolling out actual breaking changes and are usually filed by the person who owns the change.

Screenshot 2024-04-29 at 16 27 11

This was unclear to me when opening the issue, maybe the description should be updated

stephane-archer avatar Apr 29 '24 14:04 stephane-archer

Agree on the template documentation. Templates that are only intended for internal use should be documented clearly as such. Even if it means starting their name with you [Internal use only] or something.

(Unless we can differentiate the templates proposed to team members and to other users in GitHub.)

lrhn avatar May 01 '24 09:05 lrhn

The deprecation does look small when written like that. My main worry is the number of people who would suddenly get a deprecation warning, and would need to address it.

If we can dart fix it, is not even a bad idea. I don't expect a lot of users to have internalized the name to the point where they'll keep writing log instead of ln forever, and curse us every time it happens.

That said, other languages that we generally try to be somewhat consistent with use log for the natural logarithm: Java, C#, and JavaScript. (JavaScript copied Java, so no surprise there.)

Those languages also have log2 and log10 functions for listings with other bases, which we do not. We have ln2 and ln10 contestants that you can multiply or divide by instead, which means that if we switch to ln for the natural logarithm, we're locked out of having accompanying ln2 and ln10 functions. (Not that the constant names are consistent to begin with.)

All in all, it's probably better to keep what we have. Even the opportunity cost of implementing a dart fix for it isn't really worth it, the issue is too rare and too early worked around.

And if we want to add log2 and log10 functions in the future, we still can with the current names.

lrhn avatar May 01 '24 09:05 lrhn

My workaround:

import 'dart:developer' as dev;

foo() {
  int i = 0;
  dev.log(i)
}

Is that standard?

charneykaye avatar Oct 16 '24 07:10 charneykaye