-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
refactor: improve payload type #954
Conversation
Important Review SkippedReview was skipped due to path filters Files ignored due to path filters (10)
You can disable this status message by setting the Tip Early Access Features
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
I think it might be a bit to broad, it results i cue numbers becoming floats and so on |
@alex-Arc Agreed, I dont suggest we merge this. I think we should try and find a way to keep the user desired format. Are there any apps that do this nicely? |
Looks like you are only allowing one argument in the |
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 stuff, just left some small suggestions for the code style. Behaviour seems good to me, lets see with @alex-Arc that is more in touch with this part of the application
looks to work good |
@alex-Arc I won't be able to get to it until later today if you want to do the tests, if I don't see anything by this evening I'll jump back and split the regex into that regex util and add the tests |
@jwetzell then what if the payload is |
@alex-Arc is this templating specific to OSC arguments or is it used in multiple places? I'm thinking if the templating could be setup (maybe a boolean passed into it) to return empty string results as a double quoted empty string |
we use the same template parser for all the integrations, but we could maybe make a version for OSC |
This is a quick test to solve the issue described by the user
There are two points of improvement here
For the user defined case, where we want to send two integers
1
1
, I believe the easiest case is to wrap this in an array[1,1]
, so that we can treat the arguments individuallyWe should take some time investigating to make sure there are no unintended side effects
Closes #953