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

Implement the "pretty printing feature", issue #20 #22

Closed
wants to merge 34 commits into from

Conversation

@lelit
Copy link
Contributor

lelit commented Jul 31, 2017

This implements a PrettyPrinter class with a set of node-specific functions that serializes a syntax tree back into a textual representation: it currently handles all basic DML statements. To test the functionality, it compares the syntax tree of the original SQL to that of the reformatted representation.

It also implements several new Node types: as requested in #6, I'm willing to rebase all these commits into two separate branches, if you prefer, but I'd like to have your bless before going that route. In that case, given that all new Nodes are already heavily tested by this functionality, I'd like to know if you think I should still write standalone test cases for them.

Thanks in advance for the review!

lelit added 30 commits Jul 30, 2017
Some nodes are extracted as plain string values, but I need to discriminate them to properly
write back their textual form.
Also, augment some of the existing classes with the symbolic constants to represent the enums
used for particular scalar attributes.
Lot's of things are still missing (right now, only the SELECT statement is covered), but the
basic infrastructure is there and seems to work reasonably well.

Obviously the "pretty is in the eye of the beholder", and the formatting style is currently
very biased on my own taste, but the idea is that it could be driven by a set of options, once
the dust settles.
I initially assumed that complex printers would require some context to be carried around, so I
started by implementing them as classes. That turned out to be untrue, and plain functions are
up to the task.
Ideally a list of items could be written on a single line, when its length does not exceed some
threshold. This was just an attempt to implement that behaviour, but I didn't like the result.
@lelit

This comment has been minimized.

Copy link
Contributor Author

lelit commented Jul 31, 2017

Uhm, I used pytest as recommended in the README, I didn't realize it's not being used by Travis...

@lelit

This comment has been minimized.

Copy link
Contributor Author

lelit commented Jul 31, 2017

Is there any particular reason to prefer the standard unittest runner over pytest?

@alculquicondor

This comment has been minimized.

Copy link
Owner

alculquicondor commented Aug 1, 2017

There is no particular reason. Please feel free to add the dependency in travis if needed. Also, since we are going to do separate PRs for the Nodes and then the PP feature, we should have tests for the former. They don't need to be very complex.

@@ -35,6 +35,26 @@ def val(self):
return self.str


class Literal(String):
@@ -85,7 +93,7 @@ class UpdateStmt(Statement):

def __init__(self, obj):
self.relation = build_from_item(obj, 'relation')
self.target_list = build_from_item(obj, 'targetList')
self.target_list = build_from_item(obj, 'targetList', 'ResTargetUpdate')

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2017

Owner

why is this override needed?

@@ -14,6 +17,11 @@ def __str__(self):

class SelectStmt(Statement):

OP_NONE = 0
@@ -159,8 +167,8 @@ def tables(self):
class CommonTableExpr(Node):

def __init__(self, obj):
self.ctename = obj.get('ctename')
self.aliascolnames = build_from_item(obj, 'aliascolnames')
self.ctename = Name.from_string(obj.get('ctename'))

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2017

Owner

This class is going away

@@ -212,6 +220,10 @@ def tables(self):
return set()


class ResTargetUpdate(ResTarget):

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2017

Owner

please remove this class

self.end_offset = build_from_item(obj, 'endOffset')
self.location = obj.get('location')

def tables(self):

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2017

Owner

Are you sure about this override? See https://github.com/alculquicondor/psqlparse/blob/master/psqlparse/nodes/nodes.py#L8 and don't implement the method if you are not sure about the node containing tables (for now). Do the same for any other Node.



class RangeVar(Node):

# NB: these changed in PG 10!

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2017

Owner

maybe we should add in the readme that we are targeting postgres 9.5

@@ -11,14 +11,15 @@ def get_node_class(class_name):
return getattr(module, class_name, None)


def build_from_obj(obj):
def build_from_obj(obj, override_class_name=None):

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2017

Owner

We shouldn't be needing to override the class name if we only have the classes from postgres

@@ -0,0 +1,846 @@
# -*- coding: utf-8 -*-

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2017

Owner

TODO for me: review this code (but ideally it should be a separate PR).

@@ -0,0 +1,19 @@
# -*- coding: utf-8 -*-

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2017

Owner

I don't see this being used on the travis configuration, but maybe I'm missing something?

@lelit

This comment has been minimized.

Copy link
Contributor Author

lelit commented Aug 4, 2017

Please ignore/close this PR: as requested in #6, I extracted the additional nodes I implemented here and rebased them in a different PR (see #23). That obviously implies the commits presented here will not be valid anymore, and will require further work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.