Skip to content

Preliminary work for a future change in outputting results that uses less memory - #780

Merged
j-bennet merged 1 commit into
masterfrom
feature/output_formatter_generator
Aug 18, 2017
Merged

Preliminary work for a future change in outputting results that uses less memory#780
j-bennet merged 1 commit into
masterfrom
feature/output_formatter_generator

Conversation

@meeuw

Copy link
Copy Markdown
Contributor

Description

Preliminary work for a future change in outputting results that uses less memory

Checklist

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

@meeuw
meeuwforce-pushed the feature/output_formatter_generator branch 9 times, most recently from 2581140 to 0afbe56CompareAugust 10, 2017 18:00
@codecov-io

codecov-io commented Aug 10, 2017

Copy link
Copy Markdown

Codecov Report

Merging #780 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #780 +/- ##
==========================================
+ Coverage 82.85% 82.91% +0.05% 
==========================================
Files 20 20 Lines 2321 2329 +8 ==========================================
+ Hits 1923 1931 +8 
Misses 398 398
Impacted FilesCoverage Δ
pgcli/packages/pgliterals/main.py79.86% <0%> (+0.28%)⬆️

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 386838b...d2ecbde. Read the comment docs.

@meeuwmeeuw mentioned this pull request Aug 10, 2017
1 task
@meeuw
meeuwforce-pushed the feature/output_formatter_generator branch from 0afbe56 to cf7e68cCompareAugust 12, 2017 07:25
@j-bennet

Copy link
Copy Markdown
Contributor

@meeuw Is this WIP or done?

@meeuw
meeuwforce-pushed the feature/output_formatter_generator branch from cf7e68c to 483895bCompareAugust 18, 2017 04:55
@meeuw

Copy link
Copy Markdown
ContributorAuthor

done, please review and merge.

@j-bennetj-bennet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by the first text_type check. Hope you can help me figure out what's going on.

Thanks!

Comment threadchangelog.rst
* Don't quote identifiers that are non-reserved keywords. (Thanks: `Joakim Koljonen`_)
* Remove the ``...`` in the continuation prompt and use empty space instead. (Thanks: `Amjith Ramanujam`_)
* Add \conninfo and handle more parameters with \c (issue #716) (Thanks: `François Pietka`_)
* Preliminary work for a future change in outputting results that uses less memory. (Thanks: `Dick Marinus`_)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in "Internal changes" section.

Comment threadpgcli/main.py
formatted = formatter.format_output(rows, headers, **output_kwargs)
first_line = formatted[:formatted.find('\n')]

if isinstance(formatted, (text_type)):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be anything other than text_type? If yes, what happens in that case?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly what this PR is about (you have a sharp eye!). In the next version of cli_helpers a generator will be outputted from format_output which yields lines without newline characters.

When this new version of cli_helpers is released these lines can be removed. See dbcli/cli_helpers#13

@meeuw
meeuwforce-pushed the feature/output_formatter_generator branch from 483895b to d2ecbdeCompareAugust 18, 2017 18:40
@j-bennet
j-bennet merged commit b136196 into masterAug 18, 2017
@j-bennet

Copy link
Copy Markdown
Contributor

👍

@j-bennet
j-bennet deleted the feature/output_formatter_generator branch August 18, 2017 22:27
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@meeuw@codecov-io@j-bennet