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

bandwhich 0.7.0 (new formula) #48504

Closed
wants to merge 1 commit into from
Closed

bandwhich 0.7.0 (new formula) #48504

wants to merge 1 commit into from

Conversation

@imbsky
Copy link
Contributor

imbsky commented Jan 3, 2020

  • 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>)?

@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 3, 2020

The current test is just to print out the version, but I think that's enough because it's a tool like MTR that's hard to test. If another test is required, I will add the proposed test.

@bayandin bayandin assigned bayandin and unassigned bayandin Jan 3, 2020
@bayandin bayandin added the new formula label Jan 3, 2020
@imbsky imbsky mentioned this pull request Jan 3, 2020
@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Jan 4, 2020

Please keep the description to just the tool usage and do improve the test.

@kontrafiktion

This comment has been minimized.

Copy link
Contributor

kontrafiktion commented Jan 4, 2020

Perhaps wait until imsnif/bandwhich#51 is solved?

@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 4, 2020

Yes, In the meantime, we will improve the description and test.

@imbsky imbsky changed the title bandwhich 0.6.0 (new formula) bandwhich 0.7.0 (new formula) Jan 5, 2020
@imbsky imbsky force-pushed the imbsky:bandwhich branch from febeb4d to 57f376e Jan 5, 2020
@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 5, 2020

bandwhich addressed an upstream bug and bumped to 0.7.0. So I updated this PR.
I understand, but MTR's fomula just prints the version. I think that's probably enough for this tool. or do you have any ideas?

@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 5, 2020

The software itself should be tested upstream, and the data output from this tool varies with the environment, making it difficult to test.

@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Jan 5, 2020

While the data might differ between environments I'd imagine that there are some things standard in the output. If that's not the case, maybe you can make it fail reliably. Version checks just aren't enough to check if the install is correct and successful.

@imbsky imbsky force-pushed the imbsky:bandwhich branch from 57f376e to c13a5f8 Jan 5, 2020
@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 5, 2020

I replaced with a new test that fail reliably. What do you think?

@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Jan 5, 2020

I think you forgot to compare the output to something.

@imbsky imbsky force-pushed the imbsky:bandwhich branch from c13a5f8 to 4e83532 Jan 5, 2020
@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 5, 2020

I made some changes. There should be no problem with this. (I hope)

@imbsky imbsky force-pushed the imbsky:bandwhich branch from 4e83532 to 995d08c Jan 5, 2020
@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Jan 5, 2020

Looks good to me, just waiting for CI. Thanks @imbsky! Without contributions like yours it'd be impossible to keep homebrew going with the high standards that users have come to expect from the project. You can feel good knowing that you've made the world a tiny bit better for homebrew users around the world! 👍 🎉

@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 5, 2020

I'm always surprised by the polite reviews of the brew team. Thank you, too!

@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 5, 2020

Yay! The test passed!

@chenrui333 chenrui333 closed this in 26dba1e Jan 5, 2020
@chenrui333

This comment has been minimized.

Copy link
Member

chenrui333 commented Jan 5, 2020

Thanks @imbsky!!

@imbsky

This comment has been minimized.

Copy link
Contributor Author

imbsky commented Jan 5, 2020

👍

@imbsky imbsky deleted the imbsky:bandwhich branch Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.