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

SPLIT Join node - support for msg.resetTimeout … #3121

Merged
merged 2 commits into from
Sep 30, 2021

Conversation

magma1447
Copy link
Contributor

@magma1447 magma1447 commented Aug 25, 2021

… to restore original timeout

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

Adds support for the join node to reset its timer to the original value, by using an input key with the message. Similar to how it already handles msg.complete and msg.reset. I chose the name msg.resetTimeout.

This is a minor patch and I also created this forum thread.

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@magma1447 magma1447 changed the title SPLIT Join node - support for payload.resetTimeout … SPLIT Join node - support for msg.resetTimeout … Aug 25, 2021
Copy link
Member

@dceejay dceejay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is resetTimeout the best / most obvious name ? To my mind reset may indicate clearing the timeout. Should it be restartTimeout or something ? thoughts ?

@@ -118,6 +118,7 @@ <h4>Manual mode</h4>
<p>A <i>timeout</i> can be set to trigger sending the new message using whatever has been received so far.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the msg.resetTimeout property would be better mentioned as part of this line, as you have to have set a timeout in order to reset / extend it.

Updated documentation differently
@magma1447
Copy link
Contributor Author

@dceejay Since this isn't really my playing field, I followed both your advises. Regarding the name, I totally agree that restartTimeout is a better name than resetTimeout.

Regarding the documentation change I am not sure I find it more readable. But I also believe that you have much more experience with how Node-red and other nodes look, so in my opinion, your word has a lot of weight here. And to be honest, I don't care too much about the details, I am just happy if the feature I wish to use makes it into mainstream.

I do want to make as good job as possible though. I hope I am doing decently, because I am not too used to work with big projects like this, and even pull requests at GitHub and so forth. If I am doing something wrong, feel free to inform me so that I can learn.

@dceejay
Copy link
Member

dceejay commented Aug 27, 2021

looking good so far :-)

@dceejay
Copy link
Member

dceejay commented Sep 13, 2021

I'm happy for this to be merged

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 67.634% when pulling f20565f on magma1447:dev into f1e7ec0 on node-red:dev.

@knolleary knolleary merged commit 5df0dae into node-red:dev Sep 30, 2021
@knolleary
Copy link
Member

Thanks @magma1447 - this will be in the 2.1 release.

I have made a small update to the help text and added a unit test for the functionality here: ec27e19

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.

4 participants