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 custom resource path. #157

Merged
merged 1 commit into from
Oct 17, 2015
Merged

Fix custom resource path. #157

merged 1 commit into from
Oct 17, 2015

Conversation

Bekt
Copy link
Contributor

@Bekt Bekt commented Oct 17, 2015

Currently engineio.Server throws an error when the resource keyword argument is provided because it is not one of the options. These changes fix the issue.

On a side note:

I spent a few minutes trying to write a test case for this but the module seems very fragile. Some of the issues I've noticed:

  • init_app() style does not work. SocketIO.on decorator uses uninitialized values
  • The line if self.server is None: self.run(app) in SocketIO.test_client doesn't make sense because the run method never initializes self.server causing various NoneType attribute errors

miguelgrinberg added a commit that referenced this pull request Oct 17, 2015
@miguelgrinberg miguelgrinberg merged commit f525fec into miguelgrinberg:v1.0 Oct 17, 2015
@miguelgrinberg
Copy link
Owner

init_app() style does not work. SocketIO.on decorator uses uninitialized values

Can you give me more details about this please?

The line if self.server is None: self.run(app) in SocketIO.test_client doesn't make sense

Yes, this was required before to compensate for some initialization happening in the run() method, but I cleaned that up and isn't necessary anymore. I'm going to remove it.

@Bekt
Copy link
Contributor Author

Bekt commented Oct 19, 2015

Thanks @miguelgrinberg. Looks like you fixed the first issue in the latest commit.

@Bekt Bekt deleted the custom-resource branch October 19, 2015 00:28
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.

2 participants