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

change WebValve.register to accept a class name instead of class #35

Merged
merged 4 commits into from
Sep 22, 2019

Conversation

samandmoore
Copy link
Member

rails 6 autoloading emits a deprecation warning for class loading during
app initialization. webvalve's register call is a culprit here, but it
doesn't have to be. this changeset makes it so that webvalve accepts a
stringified class name instead of a class and makes that all work
properly with the internals.

/domain @smudge @jmileham @effron @aburgel
/no-platform

more info on this change can be found here rails/rails#36363
the tl;dr for us is that we could remove webvalve things from the autoload paths, but that would make local dev more tedious. so, instead, we just don't load the class until after initialization.

rails 6 autoloading emits a deprecation warning for class loading during
app initialization. webvalve's register call is a culprit here, but it
doesn't have to be. this changeset makes it so that webvalve accepts a
stringified class name instead of a class and makes that all work
properly with the internals.
@nanda-prbot
Copy link

Needs somebody from @smudge, @jmileham, @effron, and @aburgel to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

@@ -1,6 +1,4 @@
class FakeTwitter < WebValve::FakeService
URL = 'http://faketwitter.test'.freeze
Copy link
Member Author

Choose a reason for hiding this comment

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

we can't do this sorta thing anymore. we'd have to put this URL into lib or some other non autoloaded path. i was never a huge fan of this approach, so i'm not worried about it.

CHANGELOG.md Outdated
@@ -9,9 +9,13 @@ and this project aims to adhere to [Semantic Versioning](http://semver.org/spec/
### Added
### Removed

## [0.9.11] - 2019-09-23
Copy link
Member

Choose a reason for hiding this comment

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

This should go in version.rb too, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I'm gonna update before merge.

@@ -2,6 +2,7 @@ language: ruby
rvm:
- 2.3.1
- 2.4.2
- 2.5.1
Copy link
Member

Choose a reason for hiding this comment

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

Should 2.6.1 go in this list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea might as well. I gotta fix the rails 6 thing to make sure we exclude < 2.5 in ci

@smudge
Copy link
Member

smudge commented Sep 22, 2019

<< domain LGTM 👍

@nanda-prbot
Copy link

Approved! 😻 👯 🎇

@samandmoore samandmoore merged commit 03a16d0 into master Sep 22, 2019
@samandmoore samandmoore deleted the sam-rails6-deprecation branch September 22, 2019 19:14
Configure one by setting the ENV variable "#{service_name.to_s.upcase}_API_URL"
or by using WebValve.register #{service.name}, url: "http://something.dev"
or by using WebValve.register "#{service_class_name}", url: "http://something.dev"

Choose a reason for hiding this comment

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

im a little late to the party with this nit but should this message have the evaluated service_class_name in quotes so no one gets confused and uses the now deprecated way of registering a service?

Choose a reason for hiding this comment

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

I guess that's handled globally in the manager class

Copy link
Member Author

Choose a reason for hiding this comment

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

it's in quotes here, no? since this string is inside a heredoc the quotes are literals

samandmoore added a commit that referenced this pull request Sep 23, 2019
* origin/master:
  change WebValve.register to accept a class name instead of class (#35)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants