Skip to content

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Apr 2, 2019

this PR adds a --path option for the highlight command.
refs #237

this is meant as a simplified initial proposal so I can get early feedback.
after we agreed on how this option should look like in the highlight-case, I will adjust the remaing 2 commands accordingly.

Todo

  • support the new option in lint-command
  • support the new option in tokenize-command

public function usageHighlight()
{
echo "Usage: highlight-query --query SQL [--format html|cli|text]\n";
echo "Usage: highlight-query [--query SQL|--path path/to/my.sql] [--format html|cli|text]\n";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initially I named this option --file, but this somehow collides with the --format option when using getopt (I guess because of the same first character or something).

to get arround this naming problem I renamed the option to --path.

If you like --file more I can do the changes, after someone guided me on how the opts-format needs to look like.

@staabm
Copy link
Contributor Author

staabm commented Apr 5, 2019

Just implemented the stdin reading case in #239

this might make this change here obsolete. please tell me your feeling whether supporting this parameter still makes sense, or whether people should just use
cat my.sql | bin/highlight-query or similar instead of bin/highlight-query --path=my.sql

@devenbansod
Copy link
Member

I'm in favor of the stdin approach in #239 (though I don't have a very strong opinion on it and could be convinced otherwise) because I find it sleeker and more pluggable over this approach.

@staabm
Copy link
Contributor Author

staabm commented Apr 5, 2019

I totally agree.

@devenbansod
Copy link
Member

Thanks for opening this PR.

I'm closing this and we'll continue the discussion at #239. 👍

@devenbansod devenbansod closed this Apr 5, 2019
@ibennetch
Copy link
Member

ibennetch commented Apr 5, 2019 via email

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.

3 participants