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

delete specified holdings and any empty clusters that result #153

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

mwarin
Copy link
Contributor

@mwarin mwarin commented Dec 14, 2021

No description provided.

@mwarin mwarin requested review from aelkiss and jsteverman December 14, 2021 20:41
bin/holdings_deleter.rb Outdated Show resolved Hide resolved
bin/holdings_deleter.rb Outdated Show resolved Hide resolved
bin/holdings_deleter.rb Outdated Show resolved Hide resolved
@aelkiss
Copy link
Member

aelkiss commented Dec 17, 2021

I think it would benefit from some documentation to indicate how to use this.

@aelkiss aelkiss requested a review from jsteverman December 17, 2021 15:11
Copy link
Contributor

@jsteverman jsteverman left a comment

Choose a reason for hiding this comment

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

OptionParser might clean this up a little bit, but not strictly necessary.

It could use some additional inline documentation, and a mention in the README.

Copy link
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

I think it would make sense to at least split out option parsing from the actual deletion (e.g. HoldingsDeleter should get a pre-parsed set of criteria).

While I agree OptionParser would simplify this I don't know if we need to do it right now if this works -- eventually I think we should consider a common CLI front-end for all our various scripts (thor is the most popular, but there are a couple other options e.g. gli)

@mwarin mwarin force-pushed the pull-delete-holdings branch from 7fba386 to 652a3e4 Compare December 23, 2021 19:26
@mwarin
Copy link
Contributor Author

mwarin commented Dec 23, 2021

Replaced homemade option parsing with optparse and squashed commits.

@mwarin mwarin requested review from jsteverman and aelkiss January 3, 2022 15:01
@mwarin mwarin merged commit b9b729a into main Jan 3, 2022
@aelkiss aelkiss deleted the pull-delete-holdings branch February 8, 2022 14:59
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