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.

Sign up for GitHub

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

Jump to bottom

Add ability to customize AppVersionCheck #168

Merged
merged 1 commit into from
Feb 4, 2021
Merged

Add ability to customize AppVersionCheck #168

merged 1 commit into from
Feb 4, 2021

Conversation

rmm5t
Copy link
Contributor

@rmm5t rmm5t commented Jan 30, 2021

What does it do?

  • Adds ability to customize AppVersionCheck with optional file, env, and transform arguments.

Related Issues

@rmm5t rmm5t mentioned this pull request Jan 30, 2021
Closed
@rmm5t
Copy link
Contributor Author

rmm5t commented Jan 30, 2021

The build failure should be remedied after merging #170 and/or #171

@rmm5t
Copy link
Contributor Author

rmm5t commented Feb 4, 2021

I'll happily rebase this branch after #170 and #171 are merged.

@anfleene
Copy link
Contributor

anfleene commented Feb 4, 2021

@rmm5t This is an awesome idea! Thanks for contributing it. I have a few inline concerns that I'll add in review. But the idea is solid and we've been talking about something like this for awhile and never got around to implementing it.

anfleene
anfleene suggested changes Feb 4, 2021
View reviewed changes
lib/ok_computer/built_in_checks/app_version_check.rb Outdated
# file - The path of the version file to check
# env - The key in ENV to check for a revision SHA
# transform - The block to optionally transform the version string
def initialize(file = "REVISION", env = "SHA", &transform)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd rather use keyword arguments for this. That way the position of the argument or their existence doesn't matter. Like this:

Suggested change
def initialize(file = "REVISION", env = "SHA", &transform)
def initialize(file: "REVISION", env: "SHA", &transform)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewschaar Agreed on the keyword args. I would have actually done that from the get-go, but it didn't look like any existing checks used them. I'll switch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to keyword args. Let me know if you'd like any other modifications. Thanks.

@rmm5t rmm5t force-pushed the 167-enhance-app-version-check branch from 41c0bff to 034cfa1 Compare February 4, 2021 18:20
@rmm5t rmm5t requested a review from anfleene February 4, 2021 18:50
@anfleene
Copy link
Contributor

anfleene commented Feb 4, 2021

Thanks!

@anfleene anfleene merged commit 2ef008b into sportngin:master Feb 4, 2021
@anfleene
Copy link
Contributor

anfleene commented Feb 4, 2021

1.18.4 is out with this improvement!

@rmm5t rmm5t deleted the 167-enhance-app-version-check branch February 4, 2021 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anfleene anfleene anfleene approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhance AppVersionCheck
2 participants
@rmm5t @anfleene