Iryna's Editor Command PR #217

amjith · 2015-04-20T16:15:36Z

This PR is a cleanup of #213 to remove extraneous commits added in due to the repo move.

I've cleaned up the commit history. I should have waited to move the repo after merging all pending PRs. That was my bad.

@j-bennet Doe this look good?

I'll review the changes and merge it in.

j-bennet · 2015-04-20T16:18:09Z

Looks good. Thanks! How did you do that? I could not figure it out!

landscape-bot · 2015-04-20T16:19:54Z

Repository health decreased by 0.32% when pulling 4006c6d on amjith:fixed_branch into 3e4d63d on dbcli:master.

2 new problems were found (including 0 errors and 2 code smells).
No problems were fixed.

amjith · 2015-04-20T16:28:02Z

Steps to reproduce:

# Clone your fork
$ git clone git@github.com:j-bennet/pgcli.git iryna_pgcli
$ cd iryna_pgcli

# Move the current working directory to the commit sha before you added your commits. 
$ git co cd1c9ea  

# Create a new branch from the current working directory state.
$ git co -b clean_state  

# Add in the main line repo as a new remote called upstream.
$ git remote add upstream git@github.com:dbcli/pgcli.git

# Bring in all the new changes from mainline. 
$ git pull upstream master

# Cherry-pick your commits on top. 
$ git cherry-pick 4315699
$ git cherry-pick a1b2d15

# Bask in the glory of success.
$ git log

# Push it to my own personal remote. My own fork is added in using the git remote add amjith-fork .....
$ git push amjith-fork fixed_branch

Open a PR from my fork to the mainline.

TADA!

There are many ways to achieve the same result, so if someone has a better way to do this, let me know.

amjith · 2015-04-21T06:11:40Z

pgcli/packages/pgspecial.py

@@ -1,6 +1,7 @@
 from __future__ import print_function
 import sys
 import logging
+import click


Pulling in click as a dependency seems wrong to me. I'll explain the reasons in a longer comment.

amjith · 2015-04-21T06:23:19Z

Longer comment as promised:

pgspecial.py should be treated as a standalone package without any dependencies. My goal is to make it into a separate package that can be installed and reused by other application authors. For example, I know that Catherine Devline is interested in using pgspecial for her own database repl at some point. The original goal of that file is to implement the backslash commands that are specific to postgres database like \dt, \dv etc. So pulling in click as a dependency is not a good idea. \e and \c are weird edge cases that are not really dealing with the database.

I'm sorry about sending this negative comment later on in the review. I should have caught this at the early stage. :(

I'm open to discussing this decision to keep pgspecial.py a standalone.

j-bennet · 2015-04-21T16:08:58Z

Ok. I didn't know this was the intent. I agree that even though "external editor" is also a special command because it's a backslash command, it does not fit between the rest of them that follow the pattern of "run some specific Postgres queries and return data". I see two ways of solving this:

Pull this command out into main.py (which already has click dependency). This feels ugly, and I don't think that main.py should deal with commands at all.
Pull this command out into a new module (perhaps iocommands.py ?) that would be used by main.

What do you think?

amjith · 2015-04-21T17:24:24Z

It's admirable to have a goal to keep main.py as clean as possible and not deal with commands at all. But I've already set the precedent (a wrong one) to deal with \c. So if you're feeling up to it, I'm all for moving it into a separate file and compartmentalizing it. So main will just make call to these packages.

j-bennet · 2015-04-21T17:25:50Z

Then I will move both \c and \e there.

j-bennet · 2015-04-21T23:07:12Z

@amjith So looks like \c has too much going on (setting internal state of pgexecute) to pull it out. But editor command is now separate from the pgspecial. Perhaps you can add https://github.com/j-bennet/pgcli/commit/9c215d25ea9d3d523929e6d58141e73b9f76fd99 to this PR.

landscape-bot · 2015-04-22T20:22:33Z

Repository health increased by 0.15% when pulling 4231545 on amjith:fixed_branch into 3e4d63d on dbcli:master.

3 new problems were found (including 0 errors and 3 code smells).
3 problems were fixed (including 0 errors and 3 code smells).

amjith · 2015-04-22T20:25:35Z

pgcli/packages/iospecial.py

+    Is this an external editor command?
+    :param command: string
+    """
+    return command.strip().startswith('\e')


Let's change this to endswith('\e').

j-bennet · 2015-04-22T20:32:53Z

@amjith I already looked into changing this to "endswith", and it's not as simple as it seems. I'll explain later if you like, but it seems that it would require some more significant changes in main module. I was thinking perhaps this (smaller) change can be implementation stage 1, and I can add a stage 2 later.

amjith · 2015-04-22T21:11:37Z

I've added a new commit to do what I had in mind. You're right it wasn't as straight forward as I explained in the comments.

So would yo mind reviewing the latest commit? I've checked it manually and it seems to work as expected.

landscape-bot · 2015-04-22T21:29:40Z

Repository health increased by 0.35% when pulling 5654cd1 on amjith:fixed_branch into 3e4d63d on dbcli:master.

3 new problems were found (including 0 errors and 3 code smells).
4 problems were fixed (including 0 errors and 4 code smells).

j-bennet · 2015-04-22T21:33:09Z

@amjith Interesting... I remember seeing some odd behavior when I changed this to "endswith", in multiline mode it didn't accept "select *\e", and wanted ";". I had to invent a workaround for that. I have no time to look now, but will check later.

j-bennet · 2015-04-22T23:25:08Z

@amjith I don't see that weird behavior anymore, seems to work fine. I'll check on mac (only verified on linux), but I think you're good to go.

j-bennet · 2015-04-22T23:54:51Z

I reproduced the problem that I was talking about on my macbook. I type in the query and \e, and instead of running the editor command, the CLI just goes to a new line and expects more input. The weirdest thing is - it did not happen on my Linux Mint.

amjith · 2015-04-23T05:21:42Z

That is a good catch. I'm surprised that it didn't happen in Linux. I know the reason why. I'll have to think about a good way to fix it.

landscape-bot · 2015-04-23T07:11:21Z

Repository health increased by 0.36% when pulling a52aab7 on amjith:fixed_branch into 3e4d63d on dbcli:master.

3 new problems were found (including 0 errors and 3 code smells).
4 problems were fixed (including 0 errors and 4 code smells).

amjith · 2015-04-23T07:30:08Z

While adding the multi-line case I discovered another corner case for \e.

Details: https://github.com/dbcli/pgcli/pull/217/files#diff-38f3aeda57742756afa677ca09dcea43R205

I've refactored the code. It probably needs more tests. But I'd appreciate another pair of eyes to check my work.

landscape-bot · 2015-04-23T08:04:30Z

Repository health increased by 0.24% when pulling 4adb83c on amjith:fixed_branch into 3e4d63d on dbcli:master.

4 new problems were found (including 0 errors and 4 code smells).
4 problems were fixed (including 0 errors and 4 code smells).

j-bennet · 2015-04-23T18:27:50Z

@amjith I have two suggestions to this:

587c5a3
b3c5a3f

landscape-bot · 2015-04-24T05:26:41Z

Repository health increased by 0.26% when pulling 2acc831 on amjith:fixed_branch into 3e4d63d on dbcli:master.

4 new problems were found (including 0 errors and 4 code smells).
4 problems were fixed (including 0 errors and 4 code smells).

amjith · 2015-04-24T05:27:54Z

pgcli/packages/iospecial.py

+    # The reason we can't simply do .strip('\e') is that it strips characters,
+    # not a substring. So it'll strip "e" in the end of the sql also!
+    # Ex: "select * from style\e" -> "select * from styl".
+    pattern = re.compile('(^\\\e|\\\e$)')


Good catch. 👍

amjith · 2015-04-24T06:14:53Z

Thank you for working with me on this PR. It was one of the tough commands with various corner cases.

🔑

Iryna's Editor Command PR

j-bennet added 2 commits April 20, 2015 09:11

Implemented external editor call.

dc3f094

Fixes for external editor command.

4006c6d

amjith self-assigned this Apr 20, 2015

j-bennet mentioned this pull request Apr 20, 2015

Added external editor command. #213

Closed

amjith reviewed Apr 21, 2015
View reviewed changes

Moved editor command into a separate module.

4231545

amjith reviewed Apr 22, 2015
View reviewed changes

Handle the case where \e is at the end of a statement.

5654cd1

Handle multi-line mode with \e.

a52aab7

Handle the case where \e is issued multiple times.

c597e16

Remove the iospecial import in pgexecute

4adb83c

j-bennet added 2 commits April 23, 2015 22:24

Fix for stripping in editor command.

7f68d82

Extracted handling of the editor command into method.

2acc831

amjith reviewed Apr 24, 2015
View reviewed changes

Make editor file read unicode compatible.

c58cc37

amjith added a commit that referenced this pull request Apr 24, 2015

Merge pull request #217 from amjith/fixed_branch

6aff7d9

Iryna's Editor Command PR

amjith merged commit 6aff7d9 into dbcli:master Apr 24, 2015

amjith deleted the fixed_branch branch April 24, 2015 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iryna's Editor Command PR #217

Iryna's Editor Command PR #217

amjith commented Apr 20, 2015

j-bennet commented Apr 20, 2015

landscape-bot commented Apr 20, 2015

amjith commented Apr 20, 2015

amjith Apr 21, 2015

amjith commented Apr 21, 2015

j-bennet commented Apr 21, 2015

amjith commented Apr 21, 2015

j-bennet commented Apr 21, 2015

j-bennet commented Apr 21, 2015

landscape-bot commented Apr 22, 2015

amjith Apr 22, 2015

j-bennet commented Apr 22, 2015

amjith commented Apr 22, 2015

landscape-bot commented Apr 22, 2015

j-bennet commented Apr 22, 2015

j-bennet commented Apr 22, 2015

j-bennet commented Apr 22, 2015

amjith commented Apr 23, 2015

landscape-bot commented Apr 23, 2015

amjith commented Apr 23, 2015

landscape-bot commented Apr 23, 2015

j-bennet commented Apr 23, 2015

landscape-bot commented Apr 24, 2015

amjith Apr 24, 2015

amjith commented Apr 24, 2015

Iryna's Editor Command PR #217

Iryna's Editor Command PR #217

Conversation

amjith commented Apr 20, 2015

j-bennet commented Apr 20, 2015

landscape-bot commented Apr 20, 2015

amjith commented Apr 20, 2015

amjith Apr 21, 2015

Choose a reason for hiding this comment

amjith commented Apr 21, 2015

j-bennet commented Apr 21, 2015

amjith commented Apr 21, 2015

j-bennet commented Apr 21, 2015

j-bennet commented Apr 21, 2015

landscape-bot commented Apr 22, 2015

amjith Apr 22, 2015

Choose a reason for hiding this comment

j-bennet commented Apr 22, 2015

amjith commented Apr 22, 2015

landscape-bot commented Apr 22, 2015

j-bennet commented Apr 22, 2015

j-bennet commented Apr 22, 2015

j-bennet commented Apr 22, 2015

amjith commented Apr 23, 2015

landscape-bot commented Apr 23, 2015

amjith commented Apr 23, 2015

landscape-bot commented Apr 23, 2015

j-bennet commented Apr 23, 2015

landscape-bot commented Apr 24, 2015

amjith Apr 24, 2015

Choose a reason for hiding this comment

amjith commented Apr 24, 2015