Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

Fix error in json parsing #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZhKostev
Copy link

Fix for "uninitialized constant TelegramBot::ApiResponse::ResponseError" + add spec for tests

@eljojo
Copy link
Owner

eljojo commented Jul 24, 2018

Thanks for opening up the PR and adding tests for this.

My thoughts:

  • I'd prefer if we stick to minitest, which is installed but currently has no tests in it.
  • it's unclear to me how this actually solves the problem, since you're just renaming the constant. Can you expand on how this fixes the problem?
  • I'd assert that the proper exception is being raised (with fail_silently: false), instead of asserting that no exception is raised.

@ZhKostev
Copy link
Author

ZhKostev commented Jul 25, 2018

@eljojo got it. I will remove rspec and investigate more about core issue, but this issue (uninitialized constant ) introduced only with rails as I see (for Schwad this issue not exists). I guess some intersection in naming or how require chain is build. It will be in a couple of days (some other work to do)

@eljojo
Copy link
Owner

eljojo commented Jul 25, 2018

Sure, take it easy.
I've been busy travelling these days, but I'll try to take a look over the weekend.

I don't think rails has something to do with it. My hunch is that ruby 2.5 is messing with things, but @Schwad not being able to reproduce is baffling.

It's also fine if we don't fix this unless @vpereira provides a more straightforward way for us to reproduce this error.

@ZhKostev
Copy link
Author

@eljojo basically I experienced this error on ruby 2.4 and rails 5.2. This is fix for me as well

@Schwad
Copy link
Collaborator

Schwad commented Jul 26, 2018

@ZhKostev aha thanks for this info! I will run same test but within rails c in a Rails 5.2 app

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants