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

List text search configs \dF #76

Merged
merged 13 commits into from
Dec 12, 2018

Conversation

igncampa
Copy link
Contributor

@igncampa igncampa commented Dec 3, 2018

Should close dbcli/pgcli#887 when it's done

adds the command \dF to list text search configurations

pgspecial/dbcommands.py Outdated Show resolved Hide resolved
@igncampa
Copy link
Contributor Author

igncampa commented Dec 5, 2018

Ok it's done now. Works beautifully. In theory I rebased and then had to force-push. I'm still fairly new to git so I'm always a bit nervous about force-pushing...

@j-bennet
Copy link
Contributor

j-bennet commented Dec 5, 2018

I get an error on dF+:

irina@/tmp:test> \dF+
local variable 'schema' referenced before assignment

@igncampa
Copy link
Contributor Author

igncampa commented Dec 5, 2018

So much for saying it works beautifully... Lesson learned

@igncampa
Copy link
Contributor Author

igncampa commented Dec 6, 2018

I'm writing tests for these. Should be done again by the end of the day.

@igncampa
Copy link
Contributor Author

igncampa commented Dec 7, 2018

This PR is ready.

Unrelated question: if I see something worth improving/changing, should I open an issue first and then a PR or should I go straight for a PR and explain the rationale there?

@j-bennet
Copy link
Contributor

j-bennet commented Dec 7, 2018

No need to open the issue just for the sake of it, just do the PR.

@j-bennet
Copy link
Contributor

j-bennet commented Dec 7, 2018

The unit tests on your branch fail on pep8 checks. How to fix:

pip install -r requirements-dev.txt
pep8radius master --docformatter -i
# git add and commit changes

@igncampa
Copy link
Contributor Author

igncampa commented Dec 7, 2018

Thanks for the heads-up. Done.

@codecov-io
Copy link

codecov-io commented Dec 7, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@de761c6). Click here to learn what that means.
The diff coverage is 85.1%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #76   +/-   ##
=========================================
  Coverage          ?   54.25%           
=========================================
  Files             ?        6           
  Lines             ?     1047           
  Branches          ?        0           
=========================================
  Hits              ?      568           
  Misses            ?      479           
  Partials          ?        0
Impacted Files Coverage Δ
pgspecial/dbcommands.py 55.65% <85.1%> (ø)

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 de761c6...0393ac7. Read the comment docs.

@j-bennet
Copy link
Contributor

j-bennet commented Dec 8, 2018

Very nice! I ran into one case where things are still breaking:

(pgcli) --- src/pgspecial ‹igncampa-list-text-search-configs› » pgcli test
irina@/tmp:test> \dF+ swed
local variable 'is_special' referenced before assignment

I'm not providing a correct pattern, but we still want to handle this case.

Looks like you aaalmost there!

@igncampa
Copy link
Contributor Author

igncampa commented Dec 8, 2018

I could really use some insight as to why this is happening. I know the issue is raised by pgcli and not pgspecial, but giving pgcli a read I cannot understand how this could be happening. My intuition tells me it has to do with nested generators, but I'm really lost here.

@j-bennet
Copy link
Contributor

j-bennet commented Dec 9, 2018

@igncampa Yes, it must be a problem with generator logic somewhere. I'll try to take a look as soon as I can.

@j-bennet
Copy link
Contributor

I think dbc543f fixes the problem.

@igncampa
Copy link
Contributor Author

Yep, you're right. I tested it on multiple environments and it does work wonderfully now. Thanks!

@j-bennet
Copy link
Contributor

Ok, I'm happy with this PR. I'll have a new release out soon.

Thank you!

🍫

@j-bennet j-bennet merged commit 61c5c87 into dbcli:master Dec 12, 2018
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.

\dF does not work to list text search configurations
3 participants