-
Notifications
You must be signed in to change notification settings - Fork 281
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
Make export TransactionOptions happen #392
Conversation
@tynes -Is this awful? Can i have this? |
Can you expand on your use case a bit? The options are internal to the http module. |
Ah - excuse me. I was in a headspace that made me feel like this would be an intended pattern. Im building a plugin and would like to add a new route. Im copy-pasting a handler (in essence) and it would make use of this class - I had naively assumed it (meaning the class) would be exported from somewhere. |
Probably worth creating a separate issue on how to handle exporting a class' options it's not possible to use both |
I don't see a particular problem with it as a similar pattern is done elsewhere in the codebase. Its also not an object that is created millions of times. @wi-ski Could you detail the new route you are working on? You also have a linting error |
I agree with @tynes Consider the style used here: Lines 1923 to 1932 in 98b33cc
Also yeah please add the |
7d179a2
to
8ba55b6
Compare
Updated 👌 |
Codecov Report
@@ Coverage Diff @@
## master #392 +/- ##
==========================================
- Coverage 60.51% 60.51% -0.01%
==========================================
Files 129 129
Lines 34750 34751 +1
Branches 5879 5879
==========================================
Hits 21029 21029
- Misses 13721 13722 +1
Continue to review full report at Codecov.
|
Tested ok in REPL:
Just fix that newline nit when you get a chance and I'll 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job @wi-ski thanks for hanging in there during review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok just sqaush these into one commit actually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wi-ski @pinheadmz @tynes I'd like us to prioritize getting #233 merged. Is this PR necessary once we get it in? I'm not sure we expose any other internal, options classes. However, any plugins that wants to create tx related endpoints could benefit from this.
7cbb45c
to
fc1b47e
Compare
@pinheadmz - Squashed 👐 Lemme know if there's anything else. |
@@ -1693,5 +1693,5 @@ function enforce(value, msg) { | |||
/* | |||
* Expose | |||
*/ | |||
|
|||
HTTP.TransactionOptions = TransactionOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dude, sorry to be super annoying about this, but the newline should stay in :-)
I think when you squashed you might've removed the commit that added the newline.
I need this for a plugin