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

script to verify hutch name #114

Merged
merged 11 commits into from
Mar 23, 2022
Merged

script to verify hutch name #114

merged 11 commits into from
Mar 23, 2022

Conversation

c-tsoi
Copy link
Contributor

@c-tsoi c-tsoi commented Mar 18, 2022

Description

Wrote a helper script to verify hutches, argument passed in is a string exits with 0 if successful, 1 if not successful.

Motivation and Context

Fixes #106.

How Has This Been Tested?

I tested the script with false hutch names and edge cases including first and last entries.

Where Has This Been Documented?

Usage.

@c-tsoi c-tsoi requested a review from ZryletTC March 18, 2022 00:14
Copy link
Contributor

@ZryletTC ZryletTC left a comment

Choose a reason for hiding this comment

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

Looking good. I'd just say clean up your comments and debug messages, then there's a bit of unnecessary code that can be taken out as well. Otherwise, good to go.

scripts/verify_hutch Outdated Show resolved Hide resolved
scripts/verify_hutch Outdated Show resolved Hide resolved
scripts/verify_hutch Outdated Show resolved Hide resolved
Copy link
Contributor

@ZryletTC ZryletTC left a comment

Choose a reason for hiding this comment

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

Looking better, keep improving 👍

scripts/verify_hutch Outdated Show resolved Hide resolved
scripts/verify_hutch Outdated Show resolved Hide resolved
scripts/verify_hutch Outdated Show resolved Hide resolved
scripts/verify_hutch Outdated Show resolved Hide resolved
scripts/verify_hutch Outdated Show resolved Hide resolved
@ZryletTC ZryletTC requested a review from ZLLentz March 22, 2022 05:44
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

The script looks solid to me! I'll give out my green checkmark.

One thing to consider: should we also add an entry to the table in the repository's README.md file?

@ZryletTC
Copy link
Contributor

Yes, @c-tsoi please add the usage for this file to the README.md (it's sorted alphabetically by script name), then I'll approve as well :)

Copy link
Contributor

@ZryletTC ZryletTC left a comment

Choose a reason for hiding this comment

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

Changing usage to same as from inside the script.
Also before we merge, could you please change the script name to verify-hutch as I believe that has been decided as the best naming convention.

README.md Outdated
<tr>
<td>verify_hutch</td>
<td>
usage: Verifies that the passed argument is a known hutch, exit 0 for true or exit for false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy and pasting from your usage within the script (and changing ${BASH_SOURCE[0]} to verify_hutch

Suggested change
usage: Verifies that the passed argument is a known hutch, exit 0 for true or exit for false.
usage: verify_hutch <hutch>
Script that returns confirmation if the passed hutch is a known/existing hutch, success indicated by exit 0, failure by exit 1.

Copy link
Contributor

@ZryletTC ZryletTC left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@c-tsoi c-tsoi merged commit 0e45694 into pcdshub:master Mar 23, 2022
@c-tsoi c-tsoi deleted the verify_hutch branch March 23, 2022 22:58
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.

Create verify_hutch or similar function/script
3 participants