OpenF2 / F2

Redefining web integration for the financial services community
Apache License 2.0
129 stars 62 forks source link

Refactor App Handlers #219

Closed montlebalm closed 7 years ago

montlebalm commented 9 years ago

This PR refactors the AppHandlers code and related tests. Its intent is to speed up the necessary tests and remove the unwanted ones (~1,300 loc). The changes to the library are minor and should retain all prior functionality.

Changes:

Minor additional changes:

Please double check my changes to src/lib/app_handlers.js and F2.log() to make sure I didn't alter existing functionality. Also, if possible, please pull down this branch and look over the AppHandler tests. See if there's anything missing or if any of the test cases should be moved around.

markhealey commented 9 years ago

Liking what I see on first pass. Thanks @montlebalm.

What about this F2.log business? It's had a storied past: #91, #123, #143. Do we keep it in 1.5?

markhealey commented 9 years ago

Do we keep F2.log in 1.5?

I meant to ask should F2.log be dep'd in 1.5? It could be replaced with proper exceptions and/or console statements as needed.

montlebalm commented 9 years ago

Thanks @markhealey for taking a first look. I put ~8 hours of work into it, so it might take a little bit to fully review.

I think depreciating F2.log would be smart unless we want to expand its role. It doesn't seem that useful if we're just wrapping the console.

montlebalm commented 9 years ago

@markhealey @brianbaker ok to merge?

brianbaker commented 9 years ago

Overall I think this is looking good, except for the two notes I added inline in the code.

montlebalm commented 9 years ago

@brianbaker I made some changes to fix the issues you pointed out. See anything else?