Skip to content
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

Suggest columns for ORDER BY and DISTINCT (fixes #685) #686

Merged

Conversation

@owst
Copy link
Contributor

@owst owst commented Apr 23, 2017

Description

Having typed an alias name in an ORDER BY or (SELECT) DISTINCT clause,
the alias was not taken account of, and the completion simply listed all
columns. This change fixes that, and makes the autocompletion behave the
same as in SELECT and WHERE clauses.

Checklist

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).
@owst owst force-pushed the column_completion_for_order_by_and_distinct branch 2 times, most recently from 22b5dce to 8bd2227 Apr 23, 2017
@owst owst changed the title Suggest columns for ORDER BY and DISTINCT (fix #685) Suggest columns for ORDER BY and DISTINCT (fixes #685) Apr 23, 2017
@codecov-io
Copy link

@codecov-io codecov-io commented Apr 23, 2017

Codecov Report

No coverage uploaded for pull request base (master@a238f0a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #686   +/-   ##
=========================================
  Coverage          ?   79.05%           
=========================================
  Files             ?       23           
  Lines             ?     2535           
  Branches          ?        0           
=========================================
  Hits              ?     2004           
  Misses            ?      531           
  Partials          ?        0
Impacted Files Coverage Δ
pgcli/packages/sqlcompletion.py 97.84% <ø> (ø)

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 a238f0a...3d560ba. Read the comment docs.

@amjith
Copy link
Member

@amjith amjith commented Apr 25, 2017

Thanks for taking the time to make pgcli better. Sorry I haven't had time to look into this. I won't have time till end of this week, but if none of the other core devs have reviewed it by then, I'll take a look. Sorry about the delay.

Copy link
Contributor

@j-bennet j-bennet left a comment

This looks good, and seems to work as expected - I tried it out on a few cases. I don't think that def table_reference is needed though.

@@ -4,10 +4,17 @@
from pgcli.packages.parseutils.tables import TableReference
import pytest

# Returns the expected select-clause suggestions for a single-table select

def table_reference(table, schema=None, alias=None, is_function=False):
Copy link
Contributor

@j-bennet j-bennet Apr 25, 2017

Choose a reason for hiding this comment

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

TableReference instantiation is already a one-liner. What's the point of wrapping it into a function?

Copy link
Contributor Author

@owst owst Apr 25, 2017

Choose a reason for hiding this comment

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

On reflection, probably none, I think I added it before I did a bunch of line-wrapping. Of course, one can avoid passing a few parameters since table_reference has default parameters, but I don't think the method pulls its weight, I'll remove it.

@@ -213,9 +220,62 @@ def test_truncate_suggests_qualified_tables():
])
def test_distinct_suggests_cols(text):
suggestions = suggest_type(text, text)
assert suggestions ==(Column(table_refs=(), qualifiable=True),)
assert set(suggestions) == set([
Copy link
Contributor

@j-bennet j-bennet Apr 25, 2017

Choose a reason for hiding this comment

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

Well done adding tests!

Copy link
Contributor Author

@owst owst Apr 25, 2017

Choose a reason for hiding this comment

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

Wouldn't be a good PR without them...!

@owst owst force-pushed the column_completion_for_order_by_and_distinct branch from 8bd2227 to e5b5b8b Apr 25, 2017
@owst
Copy link
Contributor Author

@owst owst commented Apr 25, 2017

No worries about taking your time @amjith! Thanks for the review @j-bennet - I've addressed the comment and squashed it into one commit since it was a small tweak (I hope that's ok for this project!)

@j-bennet
Copy link
Contributor

@j-bennet j-bennet commented Apr 25, 2017

@owst Also, the line in changelog:

Completion suggestions for ORDER BY and DISTINCT clauses now take account of partially-typed identifiers as per WHERE.

is not very clear. I would suggest something like:

Fixed completions after ORDER BY and DISTINCT when using table aliases.

@owst
Copy link
Contributor Author

@owst owst commented Apr 26, 2017

Yeah, fair enough, I'm certainly not wedded to the current wording. The only thing is "fixed" to me suggests it was "broken" before, when I think it was just a missing feature. Perhaps

Completions after ORDER BY and DISTINCT now take account of table aliases.

?

Having typed an alias name in an `ORDER BY` or (`SELECT`) `DISTINCT`
clause, the alias was not taken account of, and the completion simply
listed all columns. This change fixes that, and makes the autocompletion
behave the same as in `SELECT` and `WHERE` clauses.
@owst owst force-pushed the column_completion_for_order_by_and_distinct branch from e5b5b8b to 3d560ba Apr 26, 2017
@j-bennet
Copy link
Contributor

@j-bennet j-bennet commented Apr 26, 2017

@owst Yes, that is better!

@j-bennet
Copy link
Contributor

@j-bennet j-bennet commented Apr 26, 2017

Looks good. Merging.

Thank you!

🌷

@j-bennet j-bennet merged commit bad238f into dbcli:master Apr 26, 2017
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants