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

[strformat] Update the documentation to state that the precision field works only for floats #7933

Closed
kaushalmodi opened this Issue May 31, 2018 · 13 comments

Comments

Projects
None yet
6 participants
@kaushalmodi
Copy link
Contributor

kaushalmodi commented May 31, 2018

The strformat documentation says (emphasis mine):

The 'precision' is a decimal number indicating how many digits should be displayed after the decimal point in a floating point conversion. For non-numeric types the field indicates the maximum field size - in other words, how many characters will be used from the field content. The precision is ignored for integer conversions.

The .precision field seems to do nothing for strings as seen in this example:

import strformat
let str = "abc"
echo fmt">7.1 :: {str:>7.1}"
echo fmt" 7.1 :: {str:7.1}"
echo fmt" 7.2 :: {str:7.2}"
echo fmt" 7.3 :: {str:7.3}"

Output:

>7.1 ::     abc
 7.1 :: abc
 7.2 :: abc
 7.3 :: abc

I think that the implementation behavior is the Right Thing(TM), which @data-man would also agree. So can be documentation be fixed to mention that the .precision is only effective for floats?


PS: Thank you @skilchen for quickly coming up with that PR. Hopefully doing this doc change would be much simpler.

@dom96 dom96 added the Stdlib label May 31, 2018

@kaushalmodi kaushalmodi changed the title [strformat] The .precision field does not work for strings [strformat] Update the documentation to state that the precision field works only for floats Jun 1, 2018

@skilchen

This comment has been minimized.

Copy link
Contributor

skilchen commented Jun 1, 2018

I don't agree with @data-man. It is useful to be able to limit the number of characters of a string in the formatted output e.g. to produce columns of fixed size in a simple textual report. IMHO the documentation is better than the current implementation. So i make another attempt to fix the implementation in #7941

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Jun 1, 2018

It is useful to be able to limit the number of characters of a string in the formatted output e.g. to produce columns of fixed size in a simple textual report.

Hmm, that's a valid point, not that I have needed to do that, but it's a valid use case. One might say that you could slice a string as foo[0 .. 7]. But letting the fmt formatter deal with such table prettification cases looks cleaner.

@data-man

This comment has been minimized.

Copy link
Collaborator

data-man commented Jun 1, 2018

  1. I think that precision for strings should raise an exception.

  2. Or for strings should be this documentation:
    [[fill]align][sign][#][0][minimumwidth][.maximumwidth][type]
    Maybe another symbol instead of a dot.

@skilchen

This comment has been minimized.

Copy link
Contributor

skilchen commented Jun 2, 2018

Again: i disagree with @data-man.

In strformat Nim clearly tries to mimic python's f-strings. So it is desirable to have similar behavior.

In all printf-inspired string-formatting implementations (and there are really many of them) the precision field for strings always means the maximum number of characters to output. And afaik all of them use the dot to separate precision from width. I don't understand why one should raise an exception in this case. IMHO your proposed documentation is wrong (7.3s means a maximum of 3 characters within a field of 7 characters) and the relevant current documentation is clear enough:

The 'precision' is a decimal number indicating how many digits should be displayed
after the decimal point in a floating point conversion. For non-numeric types the
field indicates the maximum field size - in other words, how many characters will
be used from the field content. The precision is ignored for integer conversions.

Btw. except in the case of the g conversion the precision field has always the meaning of "number of digits/characters" and is not related to the pretty sophisticated concepts of precision or significance as used in engineering disciplines.

From an evolutionary point of view, the printf-inspiration is the winner in programming language implementations (hopefully avoiding the dangers of format string attacks). Probably only very few people do miss something like common-lisp's incredibly powerful format. Although it would be a nice exercise for the lovers of metaprogramming, to try to reimplement common-lisp's format function and loop macro in Nim. However a more useful use of available spare time for people with talents in metaprogramming would be to copy elixir's excellent unicode support to Nim. But i digress ...

@skilchen

This comment has been minimized.

Copy link
Contributor

skilchen commented Jun 5, 2018

This issue can be closed after #7941 has been merged.
Thanks to @Varriount for taking care of my PR's!

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Jun 5, 2018

Thank you!

For future, you can have an issue auto-close on PR merge if you have a string like "Fixes URL_OF_THE_ISSUE" in the commit message.

"Fixes #ISSUE_NUMBER" also works, but that makes accessing the issue thread from terminal i.e. not browser a pain.

@kaushalmodi kaushalmodi closed this Jun 5, 2018

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Jun 6, 2018

"Fixes #ISSUE_NUMBER" also works, but that makes accessing the issue thread from terminal i.e. not browser a pain.

We always use #ISSUE_NUMBER and I don't know a terminal that cannot deal with '#'. (Nor do I want to know.)

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Jun 6, 2018

I don't know a terminal that cannot deal with '#'

I meant to say that if you are in a terminal (not a browser), you need to type the whole path to the issue manually. If the entire link is in the commit message, you can copy/paste it in the browser.

The concern is not about a terminal "handling" a # or not.

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Jun 6, 2018

Putting the full issue link also helps if for whatever reason this repo is moved to a non-GitHub repo.. the "#ISSUE_NUMBER" won't work there.. you are just handcuffing yourself more with GitHub by using those shortcuts..

@Yardanico

This comment has been minimized.

Copy link
Collaborator

Yardanico commented Jun 6, 2018

@kaushalmodi but what's wrong with this? It doesn't make sense to make "#ISSUE_NUMBER" work on non-GitHub repo because no one is planning to switch from GitHub anytime soon.

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Jun 6, 2018

@Yardanico The "switching from Github" was a side note. The main point about accessibility from a non-browser env still stands.

I access all git repos (github/lab/savannah/etc) from my editor (Emacs/Magit).. it looks like this:

image

It would look the same even if you were not using Emacs, and were looking at git logs in the terminal (as Magit is just a wrapper around CLI git).

If the #7954 in that screenshot were a URL, I could have quickly jumped to that URL in my browser (Emacs can detect URL under point). As it is just "#number", I need to switch to a browser, navigate to the issues for Nim, and type that number.

So in summary, this is just a convenience request for us terminal users who don't always access repos via github.com in a browser.

@Yardanico

This comment has been minimized.

Copy link
Collaborator

Yardanico commented Jun 6, 2018

Well, maybe it should be another way - magit should implement support for these URLs? :)

@kaushalmodi

This comment has been minimized.

Copy link
Contributor

kaushalmodi commented Jun 6, 2018

The above applies to Vim/terminal users too (I just happen to use Magit).

That said, I might just add little Elisp to my config to infer the issue URL from "#NNNN". :)

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