Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"fmt" name conflict between the new stdlib strformat and pre-existing lyro/strfmt #6958

Closed
kaushalmodi opened this issue Dec 21, 2017 · 49 comments

Comments

@kaushalmodi
Copy link
Contributor

kaushalmodi commented Dec 21, 2017

Hello,

I have been using the strfmt module by user lyro on bitbucket.

The newly introduced strformat module also has the exact same named (fmt) macro.

Now the strformat fmt mimics Python f-strings and the older strfmt fmt mimics Python .format. This is very confusing and conflicting (causes errors)! It is now not possible to import both strformat and strfmt (these two are also too similarly named) and then use both styles of fmt in the same code.

Below is an example of using the fmt from pre-existing strfmt by lyro:

import strfmt
echo "{} {}".fmt(1, 0)
echo "{} {}".fmt('a', 'b')
echo "{} {}".fmt("abc", "def")
echo "{0} {1} {0}".fmt(1, 0)
echo "{0.x} {0.y}".fmt((x: 1, y:"foo"))

which outputs:

1 0
a b
abc def
1 0 1
1 foo

Now if I do:

import strfmt # I have already done nimble install strfmt
import strformat
let s = "foobar"
echo fmt"{0}"

I get:

Hint: used config file '/home/kmodi/stow/pkgs/nim/devel/config/nim.cfg' [Conf]
Hint: system [Processing]
Hint: nim_src_Ea5wBF [Processing]
Hint: strfmt [Processing]
Hint: macros [Processing]
Hint: strutils [Processing]
Hint: parseutils [Processing]
Hint: math [Processing]
Hint: algorithm [Processing]
Hint: unicode [Processing]
Hint: streams [Processing]
Hint: strformat [Processing]
stack trace: (most recent call last)
/home/kmodi/.nimble/pkgs/strfmt-0.8.5/strfmt.nim(1251) fmt
/home/kmodi/.nimble/pkgs/strfmt-0.8.5/strfmt.nim(1237) addfmtfmt
/home/kmodi/.nimble/pkgs/strfmt-0.8.5/strfmt.nim(1188) generatefmt
nim_src_Ea5wBF.nim(7, 9) template/generic instantiation from here
/home/kmodi/.nimble/pkgs/strfmt-0.8.5/strfmt.nim(1188, 18) Error: Invalid explicit argument index: 0

Can the new strformat fmt be called just f? If not that, then atleast not fmt?

Copying @vegansk (#5600) and @bluenote10 (#6507).


Update: Fix error in mentioning what was strformat fmt vs strfmt fmt.

@GULPF
Copy link
Member

GULPF commented Dec 21, 2017

Normally you'd be able to use e.g strformat.fmt"{0}" to disambiguate, but in this case it doesn't work:

lib/pure/strformat.nim(212, 11) Error: expression 'format(0, fmtRes200003)' is of type 'string' and has to be discarded

It happens because strformat doesn't expect that a format proc with two arguments will return anything, but strfmt defines such a proc. I don't know much about meta programming, but that should be fixable?

Also: shouldn't the compiler warn that fmt is ambiguous? Is that a bug?

@kaushalmodi
Copy link
Contributor Author

I don't know much about meta programming,

Me neither.

but that should be fixable?

I hope the fix is not to disambiguate using something like strformat.fmt"{0}" in your example. The Python like .format and f-string are very basic operations, and both can be used in the same code.. Hopefully the devs decide to name the strformat.fmt differently (why not just f?).

@skilchen
Copy link
Contributor

skilchen commented Dec 21, 2017

you can write

import strformat
template f(x: untyped): untyped =
  strformat.fmt(x)

btw. fmt"{0}" is not valid in either strfmt or strformat

you write:

Now the strformat fmt mimics Python .format and the strfmt fmt mimics Python f-strings.

but effectively strformat.fmt mimics Python f-strings and strfmt.fmt mimics Python .format

strfmt is excellent, except for its hand crafted formatting of floats which is broken:

import strfmt
echo "{0:f}".fmt(1e20)

produces this linenoise:

'..--).0-*(+,))+(0(.'..--).0-*(+,))+(0(

while

import strformat
template f(x: untyped): untyped =
  strformat.fmt(x)

echo f"{1e20:f}"

produces:

100000000000000000000.000000

I still would like to see someone coming up with a pure Nim (s)printf implementation. I have translated golang's strconv to Nim but i am unable to write golang's Sprintf accepting variadic arguments of any type ...

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Dec 21, 2017

btw. fmt"{0}" is not valid in either strfmt or strformat

It should be valid in strformat.. I picked that up from its documentation:

  check fmt"{0} {s}", "0 string"

Now the strformat fmt mimics Python .format and the strfmt fmt mimics Python f-strings.

but effectively strformat.fmt mimics Python f-strings and strfmt.fmt mimics Python .format

You are proving my point.. that this is already getting confusing :) Thanks, I will fix that statement in my main post above.

strfmt is excellent, except for its hand crafted formatting of floats which is broken:

I haven't extensively used strfmt, neither am I saying that one is better than another. Both serve different functions, and I hope to use both in my code.

Are there some legal issues/politics that prevent lyro's strfmt from being brought into Nim core? Why couldn't strfmt fmt and strformat fmt (hopefully renamed to something else like f) both live in strformat?

@skilchen
Copy link
Contributor

skilchen commented Dec 21, 2017

the

check fmt"{0} {s}", "0 string"

is interesting. It works as long as you don't import strfmt. But as soon as you import strfmt it doesn't work even if you use strformat.fmt"{0} {s}"

Thats because strformat.fmt"{0} {s}" finds and wants to call the procedure with this declaration:

proc format*[T](x: T): string {.inline.}

in strfmt.

@dom96
Copy link
Contributor

dom96 commented Dec 21, 2017

There is probably an issue for this already (assuming this is an issue), but shouldn't @kaushalmodi's code cause an "ambiguous error" at compile-time? If so then that's a wider issue with macros.

As for renaming fmt to f, I support it 👍 . Reason is that its implementation follows Python's f-string implementation closely already, so I don't see a reason for this discrepancy.

@Araq
Copy link
Member

Araq commented Dec 21, 2017

There is probably an issue for this already (assuming this is an issue), but shouldn't @kaushalmodi's code cause an "ambiguous error" at compile-time? If so then that's a wider issue with macros.

Macros are subject to overloading resolution.

@dom96
Copy link
Contributor

dom96 commented Dec 22, 2017

@Araq Are you saying that there is no conflict here then?

@survivorm
Copy link
Contributor

It'll be great if the strfmt migrates into strformat, with conflicts solved as @kaushalmodi proposed

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jan 6, 2018

Frank Fischer, the author of the older strfmt library that allows writing Python-like .format functions has stopped using Nim, and doesn't plan to work on that library any more [discussion here]. It was be a waste to have such an awesome library get lost into oblivion.

Can @dom96, @Araq or someone port that library into the Nim strformat library, and make both, the Python-like .format and f-strings, work together harmoniously?

@bluenote10
Copy link
Contributor

I don't understand why we need both in the standard library? The last time I checked strfmt I ran into some general issues which is why I preferred to migrate nimboost's fmt into Nim. Can't remember the details unfortunately.

@Yardanico
Copy link
Collaborator

@kaushalmodi AFAIK strformat does almost everything that strfmt does

@ervinbosenbacher
Copy link

Maybe I can take care of this, if anyone is interested?

@GULPF
Copy link
Member

GULPF commented Jan 7, 2018

IMO there is no need for another formatter in the stdlib, it would just be unnecessary bloat. strfmt works fine as a nimble package.

That said, I think fmt in strformat should be renamed to f since it's more convenient and matches Python.

@bluenote10
Copy link
Contributor

@ervinbosenbacher 👍 if you mean fixing the situation with the name collision so that both can be imported (maybe by using a different workaround in line 209 of strformat.nim). I'm not sure if renaming fmt to f goes too far. I tried that for some time with my own old string formatting library, but I ran into too many conflicts with variables/procs named f. That was a few years ago, I think the compiler is now smarter in handling overloading resolution, but exporting a symbol as generic as f might still cause trouble here and there.

I doubt that @Araq is in favor of pulling strfmt into Nim, having yet another mini language. I'm also not convinced by that :).

@ervinbosenbacher
Copy link

I can fix the name collision problem then.

@kaushalmodi
Copy link
Contributor Author

@Yardanico

AFAIK strformat does almost everything that strfmt does

From what I see, both libraries are implementing different features from Python. The strfmt is implementing the .format function of Python, and strformat the f-string.
In the .format implemention, strings like {0} have a special meaning, that refers to the value of the first variable in the list of variables add to that function. In the f-string implemention, there is no position based replacement.. {0} literally inserts 0.

I don't see any overlap between the two libraries except for the fmt name collision.

@GULPF

IMO there is no need for another formatter in the stdlib, it would just be unnecessary bloat.

That's a subjective opinion. At least love the Python .format style of string formatting. So I'll respect the collective decision of importing that library or not.

strfmt works fine as a nimble package.

It does work fine. I just hope that it's not completely abandoned once it starts breaking because of some backward incompatible change in Nim core. The best thing to keep that package alive would be to port into Nim. The second best thing would be for someone from here to take up the package maintainership from the original author (because as I mentioned above, the original author is no longer using Nim, and so not interested in maintaining this package).

That said, I think fmt in strformat should be renamed to f since it's more convenient and matches Python.

+++1 Please! Unlike the above opinion based thought, this is factual.. Both fmt calls cannot coexist in the same file. So, please allow that to happen at the very least.

@bluenote10

I'm not sure if renaming fmt to f goes too far. I tried that for some time with my own old string formatting library, but I ran into too many conflicts with variables/procs named f.

Yes, anything else would work, but not fmt! If f doesn't fly, then how about fstr (f-string)?

@ervinbosenbacher

I can fix the name collision problem then.

Thanks!

@ervinbosenbacher
Copy link

@Yardanico ok on it. As my first contribution.

@dom96
Copy link
Contributor

dom96 commented Jan 7, 2018

Definitely let's not add yet another string formatting package into the stdlib, if you want to see strfmt maintained then maybe you should start maintaining it? :)

@Yardanico
Copy link
Collaborator

@kaushalmodi but the point of f-strings is that you don't really need position-based placement

@ervinbosenbacher
Copy link

ervinbosenbacher commented Jan 7, 2018

@dom96 yes that is what I meant. Maintaining it and make sure that its not colliding with similar libraries.
If that is ok, or everyone is happy with it, basically.

@ervinbosenbacher
Copy link

Anyway let me know, I can maintain it, bugfix it, etc. In python I am using heavily he ({}).format() facility so I am happy to contribute with the same thing to Nim.

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jan 7, 2018

@dom96 I wish I could maintain it, but I don't have enough software programming skills to do that. So the best I can contribute to Nim at this point are by opening these issues that point out issues from outside the Nim box-view of the world. Having using Python, Perl, Matlab, Emacs lisp, Verilog, and many more languages for many years, I can easily see issues like inconsistencies and UX flaws in languages. But I am not a software developer, and don't understand the implementation level detail of any of these. I want to master user-level knowledge of Nim, but issues like this one come as stumbling blocks.

@ervinbosenbacher Thanks for offering to take up maintainership of strfmt. From my last discussion with the package author, he has left that package out for the world with free license: https://bitbucket.org/lyro/strfmt/issues/11/why-not-submit-this-as-a-pr-to-nim-stdlib.

So you just need to get in touch with him, and talk you him that you are interested in taking up the maintainership if that package, and maybe forking it off to your own repo.

@kaushalmodi
Copy link
Contributor Author

To clarify on my suggestion about importing strfmt into Nim core, I didn't mean to import it as it is.. but to import only the part that implements the Python .format and bake that into the new strformat library. So I was proposing that in the end, the Nim strformat would contain both: fmt from the old strfmt and the new fmt (hopefully renamed to something like f or fstr). Then people can use whichever formatting function/macro or both, if they wished.

@dom96
Copy link
Contributor

dom96 commented Jan 7, 2018

Then people can use whichever formatting function/macro or both, if they wished.

We shouldn't have two ways to do something. There should ideally be one way. What does strformat offer that the current implementation in the stdlib doesn't?

To summarise, as far as I'm concerned, all that needs to be done for this issue is to rename fmt to f. Should be a trivial thing to do and a nice first contribution for @ervinbosenbacher.

@kaushalmodi
Copy link
Contributor Author

What does strformat offer that the current implementation in the stdlib doesn't?

I believe you are referring to the 3rd party, older strfmt library. The differences between the 2 are, I believe, same as the differences between the Python .format (older strfmt) and Python f-string (new strformat). I haven't though tested how faithfully the Nim versions implement the Python counterparts. As I said earlier, picking one of the two is purely user-level de, and sometimes readability. I have seen uses in Python where sometimes the f-string will be better, and sometimes the other.

That said, I respect the decision if the .format implementation is not ported into Nim.


But coming back to facts, the fmt clash must be fixed in whatever way possible. Nim shouldn't introduce a limitation that prevents a user from using both string formatting implementations.

Thanks!

@GULPF
Copy link
Member

GULPF commented Jan 7, 2018

@dom96 Renaming fmt doesn't solve this issue unfortunately, see my previous comment.

@dom96
Copy link
Contributor

dom96 commented Jan 7, 2018

@GULPF oh yes, I mentioned that in my comment as well (#6958 (comment)). So there are two issues here at least, apologies.


I have seen uses in Python where sometimes the f-string will be better, and sometimes the other.

So please tell us what those uses are, so that we can figure out whether f-string can be improved to support them.

@Araq
Copy link
Member

Araq commented Jan 8, 2018

Renaming fmt to f does not solve the issue! To use both format libs in the same file, use this

import strfmt except format
import strformat

template f(x: untyped): untyped =
  strformat.fmt(x)

echo f"{1e20:f}"

let s = "foobar"
echo f"{0}"

IMO that's really good enough because nobody should import both at the same time.

Araq added a commit that referenced this issue Jan 8, 2018
@dom96
Copy link
Contributor

dom96 commented Jan 8, 2018

Renaming fmt to f does not solve the issue!

That's irrelevant. It should still be renamed.

@Araq
Copy link
Member

Araq commented Jan 9, 2018

I'm not sure if renaming fmt to f goes too far. I tried that for some time with my own old string formatting library, but I ran into too many conflicts with variables/procs named f.

That's irrelevant. It should still be renamed.

So why should it be renamed? And to what name?

@dom96
Copy link
Contributor

dom96 commented Jan 9, 2018

So why should it be renamed? And to what name?

It should be renamed because it follows Python's f-string semantics (which use f"..."). It should be renamed to f.

@andreaferretti
Copy link
Collaborator

I disagree, f is such a common name. In python it works because it's somehow magically baked into the string syntax, but in Nim this would become annoying

@GULPF
Copy link
Member

GULPF commented Jan 9, 2018

@andreaferretti The compiler should be able to handle it, at least in theory. E.g these two examples works right now:

import strformat
let fmt = "abc"
echo fmt"{fmt:>5}"
import strformat
proc fmt(): int = 2
echo fmt"{fmt()}"

@andreaferretti
Copy link
Collaborator

Well, but this doesn't

import strformat

proc fmt(s: string): string = s & " formatted"

echo fmt"hello"

@kaushalmodi
Copy link
Contributor Author

@Araq

So why should it be renamed? And to what name?

Why does it have to be exactly fmt which is already used in the strfmt library? The new fmt is not a rewrite of the fmt in old strfmt library. Ironically one of the Nim issues discussions is what got me start using strfmt library, and I love it.

It's very odd that both fmt implementation cannot coexist in the same file, especially when they do entirely different things. Agreed that both do string formatting, but it's a matter of taste as to which formatting implementation one picks.

If renaming to f is not a popular option, then let's be creative.. there are so many other f* names possible.. fstr? fstring?ffmt?

And after the rename we should be able to use both string formatting implementations in Nim, right?

@dom96
Copy link
Contributor

dom96 commented Jan 9, 2018

I disagree, f is such a common name

I'm struggling to think of a single instance when I used f as a name for a procedure, or anything else for that matter.

@kaushalmodi
Copy link
Contributor Author

I'm struggling to think of a single instance when I used f as a name for a procedure.

I agree, that's sort of a Programming 101 failure. Why would anyone use single character functions in their code??

@data-man
Copy link
Contributor

data-man commented Jan 9, 2018

Why does it have to be exactly fmt which is already used in the strfmt library?

If another library has an echo function, then let's rename echo to e.

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jan 9, 2018

The strfmt library is older, much older, and used in the wild.. Just searching GitHub gives this.

On the other hand strformat is used only in a handful of projects ... Most being in the recent py2nim project, the Nim repo itself, advent_of_code_2017 repo.

I wouldn't be surprised if strfmt is also used a lot more in private projects and repos, GitLab (which cannot be globally searched), etc.

If another library has an echo function, then let's rename echo to e.

If some library now creates a new echo function, they have to figure out how their echo behaves with the presence of inbuilt echo. The same holds true if Nim tries to tread of the namespace of an existing popular library.

/twocents

@bluenote10
Copy link
Contributor

bluenote10 commented Jan 9, 2018

A function called f is not that uncommon in functional style or when writing tests. In fact, for higher order functions like map, f is the standard argument name. This could have been the source of the troubles I had when using f instead of fmt back then.

Even the Nim repo is not free of f procs:

doc/manual/generics.txt:  proc f(g: IncidendeGraph)
doc/manual/generics.txt:  proc f(g: BidirectionalGraph) # this one will be preferred if we pass a type
doc/manual/trmacros.txt:  proc f(): int =
doc/manual/trmacros.txt:  proc f(): int =
tests/async/tasyncsend4757.nim:proc f(): Future[void] {.async.} =
tests/ccgbugs/tescaping_temps.nim:proc f(t: tuple[]) = discard
tests/ccgbugs/tpartialcs.nim:proc f() =
tests/ccgbugs/trecursive_closure.nim:proc f(x: proc: MalType) =
tests/ccgbugs/tuplecast.nim:proc f(): tuple[a, b: uint8] = (1'u8, 2'u8)
tests/closure/ttimeinfo.nim:proc f(n: int): TimeInfo =
tests/concepts/t5888.nim:proc f(c: CA) =
tests/concepts/tconcepts_overload_precedence.nim:proc f(x: CustomTypeClass) =
tests/concepts/texplain.nim:proc f(o: NestedConcept)
tests/concepts/texplain.nim:proc f(o: NestedConcept) = discard
tests/js/trefbyvar.nim:proc f(a: var A) =
tests/macros/tsame_name_497.nim:proc f(o: TObj) {.macro_bug.} = discard
tests/manyloc/named_argument_bug/tri_engine/gfx/color.nim:  proc f(f: float): string =
tests/metatype/tcompositetypeclasses.nim:proc f(src: ptr TUnion, dst: ptr TUnion) =
tests/metatype/ttypebar.nim:proc f(src: ptr TFoo, dst: ptr TFoo) =
tests/misc/tvarious.nim:proc f(): int = 54
tests/openarray/tptrarrayderef.nim:proc f(a: openarray[int], b: openArray[int]) =
tests/overload/tselfderef.nim:proc f(num: int) =
tests/overload/tsystemcmp.nim:proc f(x: (proc(a,b: string): int) = system.cmp) = discard
tests/parallel/tdisjoint_slice2.nim:proc f(a: openArray[int]) =
tests/parallel/tdisjoint_slice2.nim:proc f(a: int) =
tests/parallel/tinvalid_array_bounds.nim:proc f(a: openArray[int]) =
tests/parallel/tinvalid_array_bounds.nim:proc f(a: int) = echo a
tests/parallel/tinvalid_counter_usage.nim:proc f(a: openArray[int]) =
tests/parallel/tinvalid_counter_usage.nim:proc f(a: int) = echo a
tests/parallel/tnon_disjoint_slice1.nim:proc f(a: openArray[int]) =
tests/parallel/tnon_disjoint_slice1.nim:proc f(a: int) = echo a
tests/parallel/tsimple_array_checks.nim:proc f(n: int) = echo "Hello ", n
tests/trmacros/targlist.nim:proc f(x: varargs[string, `$`]) = discard
tests/tuples/tuple_with_seq.nim:proc f(xs: seq[int]) =
tests/typerel/typalias.nim:proc f(x: var TYourObj) =
tests/types/tauto_excessive.nim:proc f(x: auto): auto =
tests/types/tauto_excessive.nim:proc f(x, y: auto): auto =
tests/vm/tvmmisc.nim:  proc f(): seq[char] =
web/news/e017_version_0_12_0.rst:    proc f(x, y: auto): auto =

@data-man
Copy link
Contributor

data-man commented Jan 9, 2018

The strfmt library is older, much older

Nim is older, much older and therefore libraries must be adapted.

@Araq
Copy link
Member

Araq commented Jan 10, 2018

It's very odd that both fmt implementation cannot coexist in the same file, especially when they do entirely different things.

They can coexist, I showed you how. If you don't read what I write here, there is little reason for me to say anything furher here.

And after the rename we should be able to use both string formatting implementations in Nim, right?

No, wrong (!!!!), because both strfmt and strformat use similar but incompatible notions of the format protocol that external types need to adhere to. And this is not easily solved, renaming fmt to f or fstr does not fix this.

@dom96
Copy link
Contributor

dom96 commented Jan 10, 2018

@Araq You seem to agree with @andreaferretti that it shouldn't be renamed to f, can I ask you why? As far as I understand there is no risk of conflicts between other identifiers named f. The upside should be obvious: Python users won't have to re-learn that f"" is fmt"" in python.

@andreaferretti
Copy link
Collaborator

How is there no conflict in presence of -say - f(x: string): string?

@dom96
Copy link
Contributor

dom96 commented Jan 10, 2018

There is in that case, just like there would be if that proc was named fmt (as things are right now). Is it really a problem to then rename the proc to something else?

@andreaferretti
Copy link
Collaborator

It is just that f is an extremely common function name, as opposed to fmt

@narimiran
Copy link
Member

Python users won't have to re-learn that f"" is fmt"" in python.

Just like they/we don't have to re-learn that append is add, add is incl, def is proc, >> is shr, etc.? :P

Agreed with @bluenote10 and @andreaferretti that f function is not that uncommon.

While I really like (and often use) Python's f-strings and I think it is a great syntax for Python, fmt just feels more Nim-like.

@narimiran
Copy link
Member

narimiran commented Jan 10, 2018

The strfmt library is older, much older, and used in the wild..

Everybody who uses Nim must be aware Nim is still at version 0.x and some breaking changes are expected to happen from time to time. (These should happen right now, rather than to either live with missed opportunities or to break things later (see Python 2 vs Python 3 endless dicussions))
And I applaud these changes if they mean we'll have a better version 1.x!

Having good string formatting (which might incorporate some things (not just fmt name) of pre-existing libraries) in the standard library is one of those changes, IMO.

Araq added a commit that referenced this issue Jan 11, 2018
@Araq Araq added the Won't Fix label Apr 22, 2018
@Araq Araq closed this as completed Apr 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests