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

Minor clean up and added node environment for jslint #7

Closed
wants to merge 6 commits into from

Conversation

formula1
Copy link

I would have liked to do however, RelayServer was hampering me from seperating the ip logic.

Additionally, I'm not exactly sure if you want to keep the event emitter private or not as that is the benefit of doing things the way your doing. However, it also adds redundant features

The Relay server itself could probably be more abstract

@greggman
Copy link
Owner

Thanks for this. One question, I don't understand the

+/*jslint node: true */

stuff. I'm using eslint. Type

grunt eslint

And it's configured to use node mode for those files already so are those jslint lines needed?

@formula1
Copy link
Author

More importantly its messy and you don't need to see it. I'm using atom which is helping me lint on the fly. But I'll run eslint as well. Why not use all the linters amirite?

@formula1
Copy link
Author

For what its worth, eslint seems quite draconian. That may be a good thing though there are times such as here it wants you to split up the variable statements which is purely a personal preference issue (which may actually prefer commas for performance, [though its impact is debatable]).

That being said, there is so much linting to do T__T

@greggman
Copy link
Owner

eslint is not draconian, my configuration of it is :P I had forgotten to check in the files that skip checking semver etc...

But, that said, I manually merged most of your changes but I couldn't use body-parser as is. It changes the behavior. Specifically if you pass bad json it just barfs, there's no catching the error. I added 2 unit tests to test post handling which caught the change in behavior. The issue is documented on SO and the body-parser repo but I looked around and didn't see a solution.

As an aside this is one of those things though that seems like overkill. body-parser is ~6000 lines of code to replace 20 lines of code. Sure it's supposed to handle some special cases but I'm not sure where any of those special cases would come up in HFT so for now I just refactored the old json parsing code into express middleware.

@formula1
Copy link
Author

Understandable :) I fully and respect your decisions. BodyParser certianly has a lot in it, though in other cases people. I'll try my best to do more tests/create more in case I submit to your repo. This is very far down the line, so the last thing I want to do break what isn't broken.

I'm sincerely excited to see this project continue to evolve!

@formula1 formula1 closed this Jan 31, 2015
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