update manual and docstrings to PrettyTables.jl

[bkamins]

I have done the manual. I will continue with docstrings tomorrow.

[bkamins]

This should be good for a review. In general you can expect that this PR does not change anything. What I do:

1. update manual
2. update all docstrings (and fix some of them that were not working)
3. remove methods that were used for printing but are not used any more (@ronisbr - here I would appreciate if you confirmed this) and remove them from internals.md
4. do the same changes as Update Categorical test #2523 (just to make sure CI passes)
5. small update of groupby tests as now we have trailing whitespace in some printed strings

In general I have used this PR to make sure all doctests pass before the release.

[bkamins]

@ronisbr - there are two things to dicsuss left in this PR. Can you please have a look at them? Thank you!

[bkamins]

I have updated the example with overwide column that caused cropping

[ronisbr]

@bkamins beside that question of float pointing alignment, is there anything else? Everything looks good to me!

[bkamins]

is there anything else?

No - all is clean. I have opened #2525 to keep track of to-do, but it is for later (you are probably best to lead these changes).

Also @nalimilan commented that the 32 char column width limit might be reconsidered, but this is for later. The question is if it is possible to do something like:

1. Have 32 char limit by default
2. But if this limit is triggered and we see that we have horizontal space then loosen it

(I assume this is tricky to do correctly, especially if you have more than one column that hits this problem, so I guess both @nalimilan and I think that it is for later)

[ronisbr]

Also @nalimilan commented that the 32 char column width limit might be reconsidered, but this is for later. The question is if it is possible to do something like:

1. Have 32 char limit by default
2. But if this limit is triggered and we see that we have horizontal space then loosen it

(I assume this is tricky to do correctly, especially if you have more than one column that hits this problem, so I guess both @nalimilan and I think that it is for later)

I am guessing here, since I really did not analyze the code, but maybe an autoexpand option for PrettyTables.jl is not that hard to implement. The column is always processed entirely and cropped when printing. Hence, I just need to change a number. The really tricky part will be to make this compatible with the rest of PrettyTables.jl API, especially when you have multiple lines in a cell or when you have omitted rows. I think this can be done, but not very quickly. I will open an issue to remind me of this.

[bkamins]

I think this can be done, but not very quickly.

Thank you. I think it is not a super pressing feature. Adding HTML and LaTeX for consistency are more of the priority (if you have the stamina for this). I hope you enjoyed the work - all JuliaData community really appreciates what you have done.

[ronisbr]

Thank you. I think it is not a super pressing feature. Adding HTML and LaTeX for consistency are more of the priority (if you have the stamina for this).

Ok! I will focus in HTML and LaTeX. We are not too far from a stable version, but there is a lot of tests to add.

I hope you enjoyed the work - all JuliaData community really appreciates what you have done.

I really did! I would like also to thank you and @nalimilan, because PrettyTables.jl is much better now after all the modifications :)

[nalimilan]

Yes the 32-char limit is much lower priority than aligning float columns to the right IMHO. If possible I'd like to get the latter in 0.22, but the former can wait.

[bkamins]

aligning float columns to the right

@nalimilan - why do you think it is that important?

[nalimilan]

For consistency with integers, and because that's what all other software does (Excel, dplyr...). But the main reason I think it deserves to be in 0.22 is that it's better to avoid breaking doctests of other packages too frequently (other than that it could perfectly wait for a later release).

[bkamins]

OK - so if this is reasonably easily doable let us wait for @ronisbr to do the update and then I will rebase and update this PR.

@ronisbr - so please let us know what is your conclusion here.

[ronisbr]

To keep the right alignment, we need to compute the field size. In the current implementation, we would need to convert the float columns to string twice (inside DataFrames and inside PrettyTables). This can decrease the performance.

The solution to this is adding an API to post process the table after the conversion of the matrix to strings. However, this will take time to design.

Hence, we have three possible solutions:

1. Process the column twice to right align the numbers properly, even if it decreases the performance;
2. Accept it as is until I can think about something to be implemented in PrettyTables; or
3. Remove the decimal alignment of floating point columns until I can solve this issue.

[bkamins]

Accept it as is until I can think about something to be implemented in PrettyTables

@nalimilan - I vote for this option. I would prefer @ronisbr to design a general solution for alignment in PrettyTables.jl first so that we do not have to use hacks in DataFrames.jl. As for option 3 - I think that decimal alignment is a priority, so I would rule it out.

Two side comments:

1. this issue is mostly visible for columns with very wide labels. for narrow labels it is almost invisible.l
2. Also when we do cropping of the last column actually having the output left aligned is better (as you see "something" at least)

Finally - we consider display changes non breaking, and such a change in the future, while requiring depending packages to update their docstrings is minor (even if these packages do not update the docstrings the difference will be minimal visually).

So in summary - given the situation I vote for leaving things as they are now, as @ronisbr already put massive work into these things. We can update it later when @ronisbr updates PrettyTables.jl to allow for flexible alignment changes.

@nalimilan - but if you would still prefer solution #1 then I understand it is doable, and I am OK with waiting till the workaround is implemented before 0.22 release, so please let us know how you see this.

[nalimilan]

Thanks for investigating this @ronisbr. With the goal of minimizing disruptions for users (because doctests can make tests fail even if printing is not part of the API), I'd be inclined to try solution 1 if you feel like it's doable reasonably soon and without doing too much work that you'll have to undo once we have a better solution. But if you feel it's not worth it then we can change that after 0.22.

[bkamins]

OK - so if @ronisbr agrees let us go for option 1 that is less performant (doing floating processing twice) and hope that in the future PrettyTables.jl is upgraded to do if faster.

@ronisbr - is this OK with you?

If yes - can you please open a PR with the fix? Then I will rebase this PR after the other is merged (this means that in your PR only update tests and docstrings for show - all other things will be handled in this PR).

(an alternative would be to merge this PR and then you could update only the outputs that get changed after your fix - we can go also this way if you would prefer it)

[ronisbr]

I think I can take a look by this weekend! Then I let you know if the downsides are that bad 😄

I prefer submit a PR and then you update this one, if you don’t mind.

[bkamins]

I prefer submit a PR and then you update this one, if you don’t mind.

Sure - no problem. So I am waiting for your PR. Thank you!

[bkamins]

@nalimilan + @ronisbr I have updated this PR after floating point alignment change. I have caught another case of NaN that is special, but it is aligned like missing and I think it is OK.

Hopefully this should be good to merge. I have bumped version to 0.22 here so when this PR is merged I will make a release (unless you object 😄).

[bkamins]

Thank you! We are going for 0.22 release now!