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

Remove some configuration options #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

knz
Copy link
Contributor

@knz knz commented Apr 12, 2019

This PR removes the ability to customize keyword casing, simplify
expressions and change the alignment mode.

Instead it proposes just two modes:

  • the default "compact" mode uses "full" alignment and simplificaiton
  • the optional "expanded" mode uses no alignment and does not simplify
    expressions

Example, "compact":

  SELECT count(*) AS count, winner, counter * 60 * 5 AS counter
    FROM (
			SELECT winner, round(length / (60 * 5)) AS counter
			  FROM players
			 WHERE build = $1 AND (hero = $2 OR region = $3)
         )
GROUP BY winner, counter;

Example, "expanded":

SELECT
	count(*) AS count, winner, counter * (60 * 5) AS counter
FROM
	(
		SELECT
			winner, round(length / (60 * 5)) AS counter
		FROM
			players
		WHERE
			build = $1 AND (hero = $2 OR region = $3)
	)
GROUP BY
	winner, counter;

This change is Reviewable

This PR removes the ability to customize keyword casing, simplify
expressions and change the alignment mode.

Instead it proposes just two modes:

- the default "compact" mode uses "full" alignment and simplificaiton
- the optional "expanded" mode uses no alignment and does not simplify
  expressions

Example, "compact":

```
  SELECT count(*) AS count, winner, counter * 60 * 5 AS counter
    FROM (
			SELECT winner, round(length / (60 * 5)) AS counter
			  FROM players
			 WHERE build = $1 AND (hero = $2 OR region = $3)
         )
GROUP BY winner, counter;
```

Example, "expanded":

```
SELECT
	count(*) AS count, winner, counter * (60 * 5) AS counter
FROM
	(
		SELECT
			winner, round(length / (60 * 5)) AS counter
		FROM
			players
		WHERE
			build = $1 AND (hero = $2 OR region = $3)
	)
GROUP BY
	winner, counter;
```
@maddyblue
Copy link
Owner

I think I understand the goals if this PR based on our hangout. It would allow us to support various use cases where users can choose a bit between readability and vertical length. I'm not yet convinced this is a knob we want to expose.

I fairly strongly think leaving the simplify option as an explicit choice is necessary because some users will not want us to change the text of their query at all, which is reasonable. For example leaving unnecessary parens can be very useful to disambiguate to human readers the order of operations since sometimes we don't have the precedence table memorized. I'm not convinced that worrying about vertical compactness deserves this top level of importance.

I'm much more worried about making most people's use of this tool great, and not worrying too much about users who don't fit in completely into that target. Like for example books or other page-break mediums. There are definitely people who care about that stuff, but I think it's better to make the experience for the relatively larger number of other users better and simpler than providing a tool that works for these other smaller groups.

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.

2 participants