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 objects from schemas not in search_path #649

Merged
merged 4 commits into from Mar 14, 2017

Conversation

koljonen
Copy link
Contributor

@koljonen koljonen commented Mar 5, 2017

I keep typing e.g. SELECT mff and getting annoyed when someschema.my_fancy_function() isn't suggested because I forgot to type the schema first. So this fixes that, for tables, views functions and datatypes.
There's a config option for getting the old behaviour.

I added some rudimentary tests. I'm working on another branch where I'll make it more convenient to run the same test for different completer configs, which will add some more test cases for this feature as well.

Sending this to you @darikg, as it's related to a bunch of my other stuff you've reviewed.

And schema-qualifying them, of course, so that for `SELECT * FROM bar`
we might suggest `buildings.barns` and for `select dopi` we might
suggest `maintenance.delete_old_personal_info()`, for example.

Controlled by a config setting, `search_path_filter`, in case anyone
prefers the old behaviour.
@darikg
Copy link
Contributor

darikg commented Mar 8, 2017 via email

Copy link
Contributor

@darikg darikg left a comment

Choose a reason for hiding this comment

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

This is nice. I left some requests for additional comments in the review.

I wonder how this interacts with the priorization? E.g. in test_smart_completion_multiple_schemata, select * from u should suggest both 'users' and 'custom.users'. Should we ensure that users is ranked ahead of the schema qualified one?


def _maybe_schema(self, schema, parent):
return None if parent or schema in self.search_path else schema

def populate_schema_objects(self, schema, obj_type):
"""Returns list of tables or functions for a (optional) schema"""
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to Returns a list of SchemaObjects...

cased_tbl = self.case(tbl)
alias = self.alias(cased_tbl, suggestion.table_refs)
def _make_cand(self, tbl, do_alias, suggestion):
cased_tbl = self.case(tbl.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment here that tbl is a SchemaObject?

@@ -747,22 +753,28 @@ def addcols(schema, rel, alias, reltype, cols):

return columns

def _get_schemas(self, obj_typ, schema):
metadata = self.dbmetadata[obj_typ]
Copy link
Contributor

Choose a reason for hiding this comment

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

A docstring here would be nice. like "Return a list of schemas from which SchemaObjects can be suggested", and note that schema is the optional user-specified schema qualification?

@koljonen
Copy link
Contributor Author

Pushed some changes.

@darikg darikg merged commit 7b14da9 into master Mar 14, 2017
@darikg
Copy link
Contributor

darikg commented Mar 14, 2017

Looks good!

@fpietka fpietka deleted the koljonen/suggest_from_all_schemas branch October 2, 2017 20:12
Sign up for free to 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.

None yet

3 participants