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

Fix eager loading due to ServeStream rename #2072

Closed

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Jun 12, 2020

ServeFile was renamed to ServeStream in #1520. Calling Grape.eager_load!
would fail with:

uninitialized constant Grape::ServeFile (NameError)

@dblock
Copy link
Member

dblock commented Jun 12, 2020

We can add a(n integration) spec for this, can't we? Calling Grape.eager_load! would fail now.

@dblock
Copy link
Member

dblock commented Jun 24, 2020

Bump @stanhu want to finish this?

@stanhu stanhu force-pushed the sh-fix-eager-loading-servefile branch 2 times, most recently from a9f1236 to 0dd6ccc Compare June 25, 2020 00:09
@stanhu
Copy link
Contributor Author

stanhu commented Jun 25, 2020

@dblock Ok, done.

.travis.yml Outdated Show resolved Hide resolved
@dblock dblock mentioned this pull request Jun 25, 2020
@stanhu stanhu force-pushed the sh-fix-eager-loading-servefile branch from 0dd6ccc to fcafe0d Compare June 25, 2020 15:41
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

There's still a need for a CHANGELOG line, this is a bug fix.

.travis.yml Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Jun 25, 2020

Errr... now looking into this I have more questions.

  • does this even need to be an integration spec? I can easily fail this with just adding this spec as is
  • what is this file supposed to do? we don't document using eager loading like this - it came from [AutoLoad] Autoload Fixes #1904, @myxoh

@dnesteryuk
Copy link
Member

This issue also happens after calling:

API.compile!

So, we might use it in the integration test.

@dblock
Copy link
Member

dblock commented Jun 28, 2020

@stanhu Want to finish so we can include this in the release? Let's include API.compile! too.

@stanhu stanhu force-pushed the sh-fix-eager-loading-servefile branch from fcafe0d to bdc1dd9 Compare June 28, 2020 13:08
@stanhu
Copy link
Contributor Author

stanhu commented Jun 28, 2020

@dblock I'm not sure if this is what you wanted, but take a look.

ServeFile was renamed to ServeStream in ruby-grape#1520. Calling Grape.eager_load!
would fail with:

```
uninitialized constant Grape::ServeFile (NameError)
```
@stanhu stanhu force-pushed the sh-fix-eager-loading-servefile branch from bdc1dd9 to 09e3156 Compare June 28, 2020 13:10
@dblock
Copy link
Member

dblock commented Jun 28, 2020

Made some changes and merged via b3adbb4.

@dblock dblock closed this Jun 28, 2020
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.

None yet

3 participants