Allow defining a json file with preferred aliases #1382
Conversation
rob-b
force-pushed
the
main
branch
2 times, most recently
from
fb73175
to
9a086ef
Compare
pgcli/pgcompleter.py
Outdated
| alias_map_file = settings.get("alias_map_file") | ||
| if alias_map_file is not None: | ||
| with open(alias_map_file) as fo: | ||
| self.alias_map = json.load(fo) |
There was a problem hiding this comment.
I wanted to flag this part as I wasn't sure of what approaches here fit the project goals
- If the file doesn't exist what should I do? Allow the crash or just handle it silently and set the map to
None? - is loading the file in the constructor a good idea? I tend not to do IO in constructors normally but since theoretically the alias file could be quite large I thought slow startup was better than a slow first query. Maintainers might think the opposite
There was a problem hiding this comment.
- If someone specified a file in the config, and it doesn't exist, it should be an error. We can assume that the user wanted the file to be picked up, and so the user will be interested in fixing the path.
- How large is "quite large"? Did you do any benchmarks with your file? If you think it's worth the effort, you could run a background thread to load these aliases, like the completion refresher does.
There was a problem hiding this comment.
I pushed a new version that wraps loading/decoding into a helper function and raises an informative error if the map_file cannot be read
My personal alias file is tiny - 5 aliases and the whole file is ~220 bytes - and so for myself at least loading is fast
In [1]: from pgcli.pgcompleter import load_alias_map_file
In [2]: from pgcli import config
In [3]: import timeit
In [4]: map_file = config.get_config()["main"]["alias_map_file"]
In [5]: t = timeit.Timer(lambda: load_alias_map_file(map_file))
In [6]: t.timeit(10_000)
Out[6]: 0.15082729199999534
I tried upping the alias file to 1000 aliases and it's still pretty fast
In [9]: t = timeit.Timer(lambda: load_alias_map_file(map_file))
In [10]: t.timeit(10_000)
Out[10]: 1.3473110840000118
How large is "quite large"?
For my own usage I can't see the file getting bigger than 200 lines (we have ~ 120 tables in prod) which is 8kb and quick to load
In [8]: t.timeit(10_000)
Out[8]: 0.3809067500000083
my caution was purely down to experience of users always doing something surprising and using features in unexpected ways.
I don't think that loading the aliases would need to be performed in a background thread
Codecov ReportBase: 84.15% // Head: 80.14% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1382 +/- ##
==========================================
- Coverage 84.15% 80.14% -4.01%
==========================================
Files 21 23 +2
Lines 2720 2166 -554
==========================================
- Hits 2289 1736 -553
+ Misses 431 430 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
pgcli/pgclirc
Outdated
| # "some_table_name": "desired_alias", | ||
| # "some_other_table_name": "another_alias" | ||
| # } | ||
| alias_map_file = "" |
There was a problem hiding this comment.
| alias_map_file = "" | |
| alias_map_file = |
| @@ -61,18 +62,38 @@ def Candidate( | |||
| normalize_ref = lambda ref: ref if ref[0] == '"' else '"' + ref.lower() + '"' | |||
|
|
|||
|
|
|||
| def generate_alias(tbl): | |||
| def generate_alias(tbl, alias_map=None): | |||
There was a problem hiding this comment.
Looks like we have no tests for this functions. I'd add one, especially now that the behavior is getting more complicated.
|
@j-bennet is there a way I can get codecov to re-run? It's reporting based on the first version of this PR and I'd be interested to see if the additional tests I've written have moved the number |
|
@rob-b I actually don't know how to bump codecov to recalculate, but I wouldn't worry about coverage report, it's not that important. Can you add your name to AUTHORS file? Otherwise I think the PR is good to merge. |
rob-b
force-pushed
the
main
branch
6 times, most recently
from
73cd0ee
to
e50eeee
Compare
At $WORK we have a lot of tables with names like `foo_noun_verb` or
`foo_noun_related-noun_verb` and so while the default aliasing is very
helpful for shortening unwieldy names we do end up with lots of aliases
like `LEFT JOIN fnv on fnv2.id = fnv.fnv2_id`
This change will allow defining a json file of preferred aliases
```
> cat ~/.config/pgcli/aliases.json
{
"foo_user": "user",
"foo_user_group": "user_group"
}
```
so the alias suggestion for `SELECT * FROM foo_user` will be `SELECT * FROM foo_user AS user`
instead of the default `SELECT * FROM foo_user AS fu`
Raise a (hopefully) helpful exception when the alias_map_file cannot be parsed or does not exist
Discussed this on the PR with a project maintainer
rob-b
force-pushed
the
main
branch
3 times, most recently
from
b4eb09c
to
b21b8ef
Compare
|
@rob-b Sorry this took forever, it got lost from my radar. Thank you for the PR! |
Description
At $WORK we have a lot of tables with names like
foo_noun_verborfoo_noun_related-noun_verband so while the default aliasing is very helpful for shortening unwieldy names we do end up with lots of aliases likeLEFT JOIN fnv on fnv2.id = fnv.fnv2_idThis change will allow defining a json file of preferred aliases
so the alias suggestion for
SELECT * FROM foo_userwill beSELECT * FROM foo_user AS userinstead of the defaultSELECT * FROM foo_user AS fuChecklist
changelog.rst.AUTHORSfile (or it's already there).pip install pre-commit && pre-commit install), and ranblackon my code.