output generator #13

meeuw · 2017-06-18T09:49:55Z

Description

output generator

Checklist

I've added this contribution to the CHANGELOG.

codecov-io · 2017-06-18T09:55:04Z

Codecov Report

Merging #13 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #13   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         268    276    +8     
=====================================
+ Hits          268    276    +8

Impacted Files	Coverage Δ
...i_helpers/tabular_output/vertical_table_adapter.py	`100% <100%> (ø)`	⬆️
...helpers/tabular_output/delimited_output_adapter.py	`100% <100%> (ø)`	⬆️
cli_helpers/tabular_output/tabulate_adapter.py	`100% <100%> (ø)`	⬆️
...i_helpers/tabular_output/terminaltables_adapter.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a44788...310088e. Read the comment docs.

tsroten · 2017-06-19T02:11:35Z

@meeuw 👍 I like this!

I'm going to look at the format number updates that Irina made and then I'll take a look at this.

tsroten · 2017-06-19T13:57:36Z

cli_helpers/tabular_output/vertical_table_adapter.py

-        output.append('\n')
-
-    return ''.join(output)
+        yield _get_separator(i, sep_title, sep_character, sep_length) + result + '\n'


@meeuw I don't think we need the newline character any longer since we're returning each record separately. It's now the client code's job to insert a newline between each record. Then you can remove the extra blank line you had to add to the tests.

tsroten · 2017-06-19T14:12:57Z

@meeuw Other than that comment I made, this looks good to me. Great job!

So, what about the tabulate and delimited output adapters?

Since this is a breaking change (mycli doesn't work without a change to support this), we're going to need to bump the version of CLI Helpers to 1.0. Plus, we need to consider pgcli as well, which will soon depend on CLI Helpers.

I'm thinking we'll release v0.2 of CLI Helpers with the format_numbers and style_output change. Along with updating mycli to depend on cli_helpers < 1.0.0. Then, we can release this in a 1.0 release.

meeuw · 2017-06-19T18:39:22Z

Hmm the changes I've made in mycli are made so it doesn't break with the current version of cli_helpers (but supports the changes in this branch). But maybe it's indeed better to only return generators/iterators from cli_helpers.

I'm all in for doing this for the 1.0 release, that will give us some time to update the other output adapters to return generators.

tsroten · 2017-06-19T18:40:21Z

@meeuw Ok, that sounds good. So, we can push the mycli change out with the next release so it will be ready whenever cli_helpers 1.0 is released.

tsroten · 2017-06-19T18:40:45Z

@meeuw I do think we should return a consistent format from CLI Helpers.

tsroten · 2017-08-09T15:08:38Z

cli_helpers/tabular_output/delimited_output_adapter.py

+    l.reset()
+    writer.writerow(headers)
+    for line in l.lines:
+        yield line[:-1]


Rather than stripping the newline by index, we can tell the csv module not to add newlines at all.

On line 32:

ckwargs = {'delimiter': delimiter, 'lineterminator': ''}

Also, lineterminator needs to be removed from line 23 since client code should not be able to specify a line ending (since we return a generator with no newlines).

tsroten · 2017-08-09T15:09:54Z

cli_helpers/tabular_output/delimited_output_adapter.py

+    writer = csv.writer(l, **ckwargs)
+    l.reset()
+    writer.writerow(headers)
+    for line in l.lines:


What about putting the loop and yield in a linewriter.readlines() function?

I don't think that's possible without leaving a loop in adapter (yield is bound to the function/method as far as I know). I've removed the loop; I assume every writerow directly calls our write function (with all the data we've supplied).

tsroten · 2017-08-09T15:16:16Z

cli_helpers/tabular_output/delimited_output_adapter.py


+class linewriter():
+    def reset(self):
+        self.lines = []


Since linewriter requires the lines attribute to work, it should create that attribute on instantiation. reset() can be used to clear the lines.

Also, in Python 3, classes are always new-style. Just to be safe we should probably make this a new-style class in Python 2: class linewriter(object):

tsroten · 2017-08-09T15:19:58Z

@meeuw I'm thinking we should make all the adapters return generators instead of just lists (for tabulate/terminaltable). What do you think?

meeuw · 2017-08-10T08:29:31Z

Hmm it doesn't add anything but I think you're right that it's better to have consistent output.

pgcli also needs changes before it can use cli_helpers, I'll try to create a PR for pgcli as well.

meeuw · 2017-08-10T18:07:22Z

dbcli/pgcli#780

tsroten · 2017-08-10T23:05:35Z

cli_helpers/tabular_output/delimited_output_adapter.py

+    l = linewriter()
+    writer = csv.writer(l, **ckwargs)
+    writer.writerow(headers)
+    yield l.line


I like this 😄

tsroten · 2017-08-10T23:12:27Z

cli_helpers/tabular_output/terminaltables_adapter.py

+        t.padding_left,
+        t.padding_right)[:3]
+    for row in t.gen_table(*dimensions):
+        yield ''.join(''.join(row))


Is there an extra join here? From what I can tell, it looks like a single join is all that's needed.

Yes, "row" is a bit confusing because it's not a row; I've copied this from:
https://github.com/Robpol86/terminaltables/blob/master/terminaltables/build.py#L151

It is called by https://github.com/Robpol86/terminaltables/blob/master/terminaltables/base_table.py#L217

I'll have a look if I can replace this by itertools.chain.from_iterable

I think they are using two joins because they join the pieces of the row together, then they join all the rows together.

We only need to join the pieces of the row together. If you put logging in you should see that the first join is returning a string, not a list. At least that's what I was seeing yesterday when I was testing it.

ah, you're right, thanks!

tsroten · 2017-08-10T23:16:24Z

@meeuw Ok, this looks good other than that last comment I left about the join. I'll look at the pgcli one next. I'd like to wait on merging this until pgcli is ready.

tsroten

This looks great! Thanks!

I'm marking it as approved, but let's not merge it until pgcli is ready.

meeuw · 2017-08-19T06:25:49Z

pgcli is ready!

meeuw force-pushed the meeuw/output_generator branch 2 times, most recently from 4371c7a to 1da434b Compare June 18, 2017 09:55

meeuw mentioned this pull request Jun 18, 2017

output via generator dbcli/mycli#461

Merged

1 task

tsroten reviewed Jun 19, 2017

View reviewed changes

meeuw force-pushed the meeuw/output_generator branch from 1da434b to 48b816f Compare June 19, 2017 18:30

meeuw force-pushed the meeuw/output_generator branch from 48b816f to 9824a19 Compare June 24, 2017 08:29

meeuw force-pushed the meeuw/output_generator branch from 9824a19 to 1bfd86b Compare July 9, 2017 11:51

meeuw force-pushed the meeuw/output_generator branch 3 times, most recently from 9312a28 to 61800c7 Compare August 6, 2017 09:27

meeuw mentioned this pull request Aug 8, 2017

Release 1.12.1 dbcli/mycli#494

Merged

tsroten reviewed Aug 9, 2017

View reviewed changes

meeuw force-pushed the meeuw/output_generator branch 5 times, most recently from 5248c36 to 4578e32 Compare August 10, 2017 06:06

tsroten reviewed Aug 10, 2017

View reviewed changes

meeuw force-pushed the meeuw/output_generator branch from 4578e32 to 782a79d Compare August 11, 2017 10:40

Output as generator

310088e

meeuw force-pushed the meeuw/output_generator branch from 782a79d to 310088e Compare August 11, 2017 14:15

tsroten approved these changes Aug 11, 2017

View reviewed changes

meeuw mentioned this pull request Aug 18, 2017

Preliminary work for a future change in outputting results that uses less memory dbcli/pgcli#780

Merged

1 task

tsroten merged commit bad6175 into master Oct 2, 2017

tsroten deleted the meeuw/output_generator branch October 2, 2017 15:48

output generator #13

output generator #13

Uh oh!

Conversation

meeuw commented Jun 18, 2017

Description

Checklist

Uh oh!

codecov-io commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tsroten commented Jun 19, 2017

Uh oh!

tsroten Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsroten commented Jun 19, 2017

Uh oh!

meeuw commented Jun 19, 2017

Uh oh!

tsroten commented Jun 19, 2017

Uh oh!

tsroten commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsroten Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsroten commented Aug 9, 2017

Uh oh!

meeuw commented Aug 10, 2017

Uh oh!

meeuw commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsroten commented Aug 10, 2017

Uh oh!

tsroten left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meeuw commented Aug 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jun 18, 2017 •

edited

Loading

tsroten Jun 19, 2017 •

edited

Loading

tsroten Aug 9, 2017 •

edited

Loading

tsroten left a comment •

edited

Loading