output via generator by meeuw · Pull Request #461 · dbcli/mycli

meeuw · 2017-06-09T12:58:25Z

Description

output via generator to prevent:

buffering of the complete output
joining and splitting the output

Checklist

I've added this contribution to the changelog.md.

tsroten · 2017-06-09T13:26:17Z

 rows, headers, format_name='vertical')

- output.append(formatted)
+ if isinstance(formatted, str) or isinstance(formatted, unicode):


You might want to try adding from mycli.encodingutils import text_type to main.py so you can use that here.

tsroten · 2017-06-09T13:57:14Z

 from .lexer import MyCliLexer
 from .__init__ import __version__

+from mycli.encodingutils import text_type


Looks like I told you wrong - you can add this to line 39 where we are already importing from encodingutils ;) Sorry about that!

tsroten · 2017-06-12T01:29:11Z


- output.append(formatted)
+ if isinstance(formatted, text_type):
+ formatted = formatted.splitlines()


When is formatted not a text_type? I think CLI Helpers always returns a text_type.

tsroten · 2017-06-12T01:44:21Z

@meeuw Thanks for looking for more ways to improve mycli 😄

So, I'm wondering, what's the advantage of operating on the line level? I'm having trouble understanding what that gets us versus what we have now.

Tabulate and Terminaltables don't return lists or iterables, so it's not like we can process the lines separately. This seems like it calls quite a bit of code for every line.

meeuw · 2017-06-12T06:21:16Z

Terminaltables is capable of returning an iterable, I'm working on a fix for cli_helpers. Terminaltables still needs to process the complete result set before outputting (to calculate the column width). But for csv/json/xml/tsv/wide output it's possible to iterate the database cursor and format/output the data without buffering the complete result set in memory.

tsroten · 2017-06-12T13:46:41Z

@meeuw Cool, sounds good to me 😄. Should I review this pull request? Or, wait until the CLI Helpers pull request?

codecov-io · 2017-06-18T09:32:13Z

Codecov Report

Merging #461 into master will increase coverage by 0.78%.
The diff coverage is 97.36%.

@@ Coverage Diff @@## master #461 +/- ##
==========================================
+ Coverage 77.83% 78.61% +0.78% 
==========================================
Files 22 22 Lines 2012 2034 +22 ==========================================
+ Hits 1566 1599 +33 + Misses 446 435 -11

Impacted Files	Coverage Δ
mycli/main.py	`79.76% <97.36%> (+2.39%)`	⬆️

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 2934073...e9b3949. Read the comment docs.

meeuw · 2017-06-18T09:59:22Z

Related to dbcli/cli_helpers#13

tsroten · 2017-06-19T14:23:46Z

- click.secho(output)
+ size = self.cli.output.get_size()
+
+ margin = self.get_reserved_space() + self.get_prompt(self.prompt_format).count('\n') + 1


I get why you're not calling output_fits_on_screen any longer (it compares the entire output), but let's rework that function so we can call it to get the margin so the margin code isn't directly in the output() function.
Since output_fits_on_screen is not used elsewhere, we can rename it and tweak it.
E.g. get_output_margin or something.

I think I accidentally removed output_fits_on_screen when I was resolving the merge conflicts, thanks for noticing, I'll try to fix it as soon as possible 😄

Hmm that's a good idea, fixed this.

tsroten · 2017-06-24T14:58:52Z


- output.append(formatted)
+ if not isinstance(formatted, types.GeneratorType):
+ formatted = formatted.splitlines()


@meeuw, so if I understand this correctly, this line won't be needed once the CLI helpers changes are made, right?

tsroten · 2017-06-24T15:01:32Z

@meeuw I'd like to get this in for our next mycli release (so mycli will be ready whenever the next CLI Helpers is released with the generator change).

I'll test it out this weekend. Other than that, the only change I think would be good is this one.

tsroten · 2017-06-24T20:07:17Z

So, one thing that's come up in my testing is that the output is always outputted to the pager, even if it fits on the screen. That's how mycli used to output results, but we recently changed it to only output via the pager if it doesn't fit (or there is the explicit pager).

meeuw · 2017-06-25T12:42:16Z

Ah you're right. I think I fixed it but I'd like to add additional tests.

meeuw · 2017-06-29T08:46:17Z

@tsroten I've added the additional tests but I think they could be a little bit more Pythonic and/or use more pytest features.

meeuw · 2017-07-02T07:26:11Z

I'm thinking of using this to check if a generator returned from cli_helpers, but I don't like to add six as a dependency.

meeuw · 2017-07-05T19:21:23Z

I've created a PR: pallets/click#815 to allow outputting to the pager using a generator

…a_generator

meeuw · 2017-07-09T18:16:38Z

@tsroten thanks, could you please update the tests or tell me how, "if the output doesn't fit and it's not going to be output via a pager, then we lose all the rows after fits is set to False?"

tsroten · 2017-07-09T19:47:00Z

@meeuw Ok, I added a test that is failing. You can replicate it by:

opening mycli
turn off the pager by running nopager.
run a query that will give you results longer or wider than the console.

You should see the results cut off because the output stops appending to buf.

meeuw · 2017-07-10T19:14:41Z

You're the best 😄 thanks a lot! Fixed.

…a_generator

…tput_via_generator

tsroten · 2017-08-07T01:41:25Z

+ for i, line in enumerate(output, 1):
+ self.log_output(line)
+ # line needs to be casted to unicode for click with Python 2.7
+ special.write_tee(text_type(line))


We need to fix whatever is not returning unicode. Do you know what it is? I know the CSV part of CLI Helpers wasn't at one point, but that should be fixed in the version that mycli requires.
If we can't fix it, then, we'll need to convert to unicode for the call to write_once() as well (line 715).

tsroten · 2017-08-07T01:45:10Z

 **output_kwargs)
- first_line = formatted[:formatted.find('\n')]
+
+ if isinstance(formatted, (str, text_type)):


I think we can drop str now that the unicode problems in CLI Helpers were fixed.

tsroten · 2017-08-07T01:45:23Z

 len(first_line) > max_width):
 formatted = self.formatter.format_output(
 cur, headers, format_name='vertical', column_types=column_types, **output_kwargs)
+ if isinstance(formatted, (str, text_type)):


Same here ^^

…a_generator

meeuw · 2017-08-07T07:17:41Z

👍 thanks, the cast isn't required anymore.

tsroten · 2017-08-07T13:26:34Z

@meeuw Good work! I think this is ready to merge. If you could just move the changelog line up to the pending release, we'll be all set.

I wonder if there is a way to word this change so that the end-user understands it.

meeuw · 2017-08-07T16:08:32Z

I've rephrased the changelog, English isn't my first language, could you please review? Feel free to change the text.

tsroten · 2017-08-08T12:55:33Z

@meeuw Ok, I reworded the changelog to be a bit shorter. It indicates that this is preliminary work to use less memory (preliminary, since CLI Helpers is still returning a string and not a generator).

If you agree with the change, then go ahead and merge this in! It'll be good to get this one in finally. Sorry it took so long!

👍 🚀

meeuw · 2017-08-08T17:07:35Z

I'm not in a hurry, thanks for your help!

amjith added the in progress label Jun 9, 2017

meeuw force-pushed the feature/output_via_generator branch from 7c9066a to 6d0e7fbCompare June 9, 2017 13:00

tsroten reviewed Jun 9, 2017
View reviewed changes

tsroten reviewed Jun 12, 2017
View reviewed changes

meeuw force-pushed the feature/output_via_generator branch 2 times, most recently from 1e49d7f to 2ad542aCompare June 18, 2017 09:28

tsroten reviewed Jun 19, 2017
View reviewed changes

tsroten reviewed Jun 24, 2017
View reviewed changes

meeuw force-pushed the feature/output_via_generator branch 4 times, most recently from 5d60435 to 162e824Compare June 24, 2017 19:30

meeuw force-pushed the feature/output_via_generator branch from 162e824 to 107be90Compare June 25, 2017 11:44

meeuw force-pushed the feature/output_via_generator branch 2 times, most recently from aecef31 to 0614e08Compare June 29, 2017 08:32

meeuw force-pushed the feature/output_via_generator branch from 0614e08 to 80185c8Compare July 2, 2017 06:27

output via generator
9e96d36

meeuw force-pushed the feature/output_via_generator branch from 80185c8 to 9e96d36Compare July 2, 2017 06:52

meeuw added 7 commits July 9, 2017 13:48

is_pager_enabled needs to be a function call
e365497

Merge remote-tracking branch 'upstream/master' into feature/output_vi…
ea0748f
…a_generator

fixup! output via generator
1aa9e09

fixup! output via generator
79629ad

fixup! output via generator
5856242

fixup! output via generator
53b2c0e

fixup! output via generator
17d60bc

Test for disabled pager with no-fit results.
ea991b4

fixup! output via generator
019aa05

meeuw added 3 commits August 6, 2017 11:38

Merge remote-tracking branch 'upstream/master' into feature/output_vi…
9828af2
…a_generator

fixup! Merge remote-tracking branch 'upstream/master' into feature/ou…
ba41e21
…tput_via_generator

fixup! output via generator
8c1193d

tsroten reviewed Aug 7, 2017
View reviewed changes

meeuw added 2 commits August 7, 2017 08:54

Remove cast to text_type which was required for py27 and fixed by Thomas
5b836c1

Merge remote-tracking branch 'upstream/master' into feature/output_vi…
27cbf6b
…a_generator

Update / rephrase changelog
6d0778b

meeuwand others added 2 commits August 7, 2017 18:32

fixup! Update / rephrase changelog
4ef958f

Simplify changelog.
e9b3949

meeuw merged commit e05cdcf into masterAug 8, 2017

meeuw deleted the feature/output_via_generator branch August 8, 2017 17:07

Uh oh!

Conversation

meeuw commented Jun 9, 2017

Description

Checklist

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 Jun 12, 2017

Uh oh!

meeuw commented Jun 12, 2017

Uh oh!

tsroten commented Jun 12, 2017

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!

meeuw commented Jun 18, 2017

Uh oh!

tsrotenJun 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!

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 Jun 24, 2017

Uh oh!

tsroten commented Jun 24, 2017

Uh oh!

meeuw commented Jun 25, 2017

Uh oh!

meeuw commented Jun 29, 2017

Uh oh!

meeuw commented Jul 2, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meeuw commented Jul 5, 2017

Uh oh!

meeuw commented Jul 9, 2017

Uh oh!

tsroten commented Jul 9, 2017

Uh oh!

meeuw commented Jul 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!

meeuw commented Aug 7, 2017

Uh oh!

tsroten commented Aug 7, 2017

Uh oh!

meeuw commented Aug 7, 2017

Uh oh!

tsroten commented Aug 8, 2017

Uh oh!

meeuw commented Aug 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jun 18, 2017•
edited
Loading

tsrotenJun 19, 2017•
edited
Loading

meeuw commented Jul 2, 2017•
edited
Loading