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

pspg 0.5 (new formula) #20686

Closed
wants to merge 1 commit into from
Closed

pspg 0.5 (new formula) #20686

wants to merge 1 commit into from

Conversation

jasonmp85
Copy link
Contributor

This adds a new formula for pspg, a pager intended for use by psql which adds features nice to have when viewing relational data. These include freezing columns and rows, consistent display of headers, and colorful themes.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@jasonmp85
Copy link
Contributor Author

OK, so if I need to run audit with --new-formula, maybe the checklist should say that.

At any rate, how do I get around the Dependency 'ncurses' may be unnecessary as it is provided by macOS; try to build this formula without it message? I know macOS provides ncurses. The problem is that it isn't built with widechar support, which results in a warning during compilation of pspg.

Given that part of pspg's usage instructions involve enabling the unicode line drawing format for psql, I'm assuming it's best to use this software with an ncurses that supports widechars.

Can I suppress this warning?

@jasonmp85
Copy link
Contributor Author

Hm, it appears the simplest around this would be to get the formula into homebrew and then add the dependency in a future build, as this rule only applies to new formulae?

@ilovezfs ilovezfs added the new formula PR adds a new formula to Homebrew/homebrew-core label Nov 16, 2017
@jasonmp85 jasonmp85 changed the title pspg 0.4 (new formula) pspg 0.5 (new formula) Nov 16, 2017
Formula/pspg.rb Outdated
url "https://github.com/okbob/pspg/archive/0.5.tar.gz"
sha256 "754d1e380d072517e9bc2c3c38785e2f19a9f927f061de9a646fd1094baa204e"

head do
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the block unless you're doing something special with head exclusively. Otherwise simply:

head "https://github.com/okbob/pspg.git"

under the sha256 line will suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, I copy-pasted from another formula that had special build dependencies for head. Oops. Fixing.

Formula/pspg.rb Outdated

def caveats; <<~EOS
Add the following line to your psql profile (e.g. ~/.psqlrc)
\\setenv PAGER pspg
Copy link
Member

Choose a reason for hiding this comment

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

Any chance this can be a 2 space indent or is the 4 space indent required in ~/.psqlrc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not required at all. I think I copy-pasted this structure from asdf's formula.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, unless there's a reason for it let's stick to a 2 space indent here. Mixing & matching indentation where not strictly required is kinda gross 🙈.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. (Though in fairness this indentation is for display purposes and not code. Homebrew might do well do establish a style for this sort of command output; right now there's vast inconsistency within the codebase (I checked postgresql for another example and it's a mess)).

end

test do
assert_match("pspg-#{version}", shell_output("#{bin}/pspg --version").chomp)
Copy link
Member

Choose a reason for hiding this comment

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

You're gonna get asked for a functionality test beyond a simple version check by @ilovezfs, so, if you can, start thinking of one 😉.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's… a pager. Kind of hard to test since most of the interaction is e.g. via keyboard (arrow keys, vim keybindings, etc.)?

I looked at e.g. nano, which calls --version and doesn't even check that its output is sane…

🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, appreciate that. Just thought I'd throw it out there in case you had ideas. Generally new formulae are expected to adhere to a higher standard than existing formulae.

Copy link
Contributor

Choose a reason for hiding this comment

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

  test do
    ENV["TERM"] = "xterm"
    assert_match /\e\[\?1049.*\e>moo/, pipe_output("#{bin}/pspg -F", "moo", 0)
  end

maybe

Copy link
Contributor Author

@jasonmp85 jasonmp85 Nov 21, 2017

Choose a reason for hiding this comment

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

My shell-fu was too weak to get there (I was worried about repeatability). I can write something up like that (indeed I already had a mini-test that wrote out a sample header/row file and printed it using pspg).

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as the match is minimal it shouldn't need much tweaking over time.

This adds a new formula for pspg, a pager intended for use by psql
which adds features nice to have when viewing relational data. These
include freezing columns and rows, consistent display of headers, and
colorful themes.
@stale
Copy link

stale bot commented Dec 12, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale No recent activity label Dec 12, 2017
@fxcoudert
Copy link
Member

@BrewTestBot test this please

@stale stale bot removed the stale No recent activity label Dec 14, 2017
@fxcoudert fxcoudert added the ready to merge PR can be merged once CI is green label Dec 14, 2017
@fxcoudert
Copy link
Member

Thanks @jasonmp85 for the pull request!

@fxcoudert fxcoudert closed this in 31f53cd Dec 14, 2017
@jasonmp85 jasonmp85 deleted the add_pspg branch December 14, 2017 17:47
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants