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

mk 1.0.2 (new formula) #74903

Closed
wants to merge 1 commit into from
Closed

mk 1.0.2 (new formula) #74903

wants to merge 1 commit into from

Conversation

ssbarnea
Copy link
Contributor

@ssbarnea ssbarnea commented Apr 10, 2021

  • 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?
  • Is your test running fine brew test <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>)?

@request-info
Copy link

request-info bot commented Apr 10, 2021

Please provide a better issue/pull request title and/or description!

@request-info request-info bot added the needs response Needs a response from the issue/PR author label Apr 10, 2021
@BrewTestBot BrewTestBot added the new formula PR adds a new formula to Homebrew/homebrew-core label Apr 10, 2021
Formula/mk.rb Outdated
url "https://files.pythonhosted.org/packages/fe/cd/fe07815319409d39c7b7506e865708caa289a82126f683b63c2e51ff3428/mk-1.0.2.tar.gz"
sha256 "6637f035ddf92a43d2daa5264ce3bfbb5c7e937a3967a1618e48f7247a6914d0"
license "MIT"
revision 1
Copy link
Member

Choose a reason for hiding this comment

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

It's a new formula so it should be revision 0, which is the default if you don't specify it.


test do
system "mk", "--version"
end
Copy link
Member

Choose a reason for hiding this comment

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

We need a test that exercises the some of the functionality of the app. Version checks or usage checks (foo --version or foo --help) are not sufficient, as explained in the formula cookbook.

In most cases, a good test would involve running a simple test case: run #{bin}/foo input.txt.

  • Then you can check that the output is as expected (with assert_equal or assert_match on the output of shell_output)
  • You can also check that an output file was created, if that is expected: assert_predicate testpath/"output.txt", :exist?

Some advice for specific cases:

  • If the formula is a library, compile and run some simple code that links against it. It could be taken from upstream's documentation / source examples.
  • If the formula is for a GUI program, try to find some function that runs as command-line only, like a format conversion, reading or displaying a config file, etc.
  • If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.
  • Same if the software requires a virtual machine, docker instance, etc. to be running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tool is excepted to be run inside cloned git repos and doing mk --help will be enough to test a lot of its functionality because almost all commands exposed by it are discovered by the tool by looking at the repository, not being "hardcoded".

Still, running it outside a cloned repo will not produce any desirable results, mainly just a message like CRITICAL Current version of mk works only within git repos. and an exit code 1.

I will try now to improve the test section a little bit but it could be useful if you can tell me how to do chdir to a folder that is already cloned.

Considering that brew already clones the homebrew-core dire, would it be ok to chdir to /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core for the purpose of the test? Is that safe or is there a better way to retrieve that folder location programately?

Copy link
Member

Choose a reason for hiding this comment

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

Could you not initialise an empty repository and then run mk there?

end

def install
virtualenv_install_with_resources(using: "python@3.9")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
virtualenv_install_with_resources(using: "python@3.9")
virtualenv_install_with_resources

3.9 is the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing using:... it from here would produce an error when trying to install:

Error: An exception occurred within a child process:
  FormulaUnknownPythonError: The version of Python to use with the virtualenv in the `mk` formula
cannot be guessed automatically because a recognised Python dependency could not be found.

If you are using a non-standard Python dependency, please add `:using => "python@x.y"`
to 'virtualenv_install_with_resources' to resolve the issue manually.

Copy link
Member

Choose a reason for hiding this comment

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

That's because python@3.9 needs to be a runtime dependency, not a build-only one.

license "MIT"
revision 1

depends_on "python@3.9" => :build
Copy link
Member

Choose a reason for hiding this comment

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

If you're installing into a virtualenv, then Python is a runtime dependency rather than build-only.

@carlocab carlocab changed the title Add formula for mk mk 1.0.2 (new formula) Apr 10, 2021
url "https://files.pythonhosted.org/packages/5c/f8/9f5e69a63a9243448350b44c87fae74588aa634979e6c0c501f26a4f6df7/blessings-1.7.tar.gz"
sha256 "98e5854d805f50a5b58ac2333411b0482516a8210f23f43308baeb58d77c157d"
end
resource "click-help-colors" do
Copy link
Member

Choose a reason for hiding this comment

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

Let's follow other formulae here and put one line of whitespace in between resource blocks.

When you make this change, please squash it with your first commit and use the commit message

mk 1.0.2 (new formula)

For future reference, please use the preferred commit-message style for homebrew/core. We put the name of the formula first in commit-message headings. Refer to the documentation for more details.

Adds formula for installing `mk` cli tool.

Reference: https://pypi.org/project/mk/
@dawidd6
Copy link
Contributor

dawidd6 commented Apr 10, 2021

==> brew audit mk --online --new-formula
==> FAILED
==> Installing 'bundler' gem
mk:
  * GitHub repository not notable enough (<30 forks, <30 watchers and <75 stars)
Error: 1 problem in 1 formula detected

Sorry but this project is currently not notable enough for inclusion in Homebrew. Consider creating a third party tap for this software and resubmit formula here when the project gains more traction.

@dawidd6 dawidd6 closed this Apr 10, 2021
@dawidd6 dawidd6 added notability Project is not notable enough for inclusion and removed needs response Needs a response from the issue/PR author labels Apr 10, 2021
@ssbarnea
Copy link
Contributor Author

I can understand that rule and the limits seem reasonable to me. What is interesting is that yesterday I run the online check and it passed without reporting the lack of popularity issue.

@github-actions github-actions bot added the outdated PR was locked due to age label May 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
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 notability Project is not notable enough for inclusion outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants