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

Assign ~thread.thid with thread_id value #2261

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

usingtechnology
Copy link
Contributor

The previous assignment used a value from a field that had not been assigned yet. So, use the correct thread_id value for the thid.

Fixes #2259

The fix originally discussed in #2259 is too aggressive and after reviewing the code it is clear that there can be empty ~thread and they are expected to be dealt with by the receiver. In this case, we were attempting to set the thid but just using an unassigned value.

usingtechnology and others added 3 commits June 8, 2023 17:03
The previous assignment was using a value from a field that had not been assigned yet.

Signed-off-by: Jason Sherman <[email protected]>
dbluhm
dbluhm previously approved these changes Jun 9, 2023
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

The new dev dependency is interesting. I usually just use the random package directly, telling it to select values from ascii (or something like that). But I'm happy with this change.

@usingtechnology
Copy link
Contributor Author

I can remove the dev dependency, I really only added it to kick off the tests again (failed in GH) as I do not have access to manually restart the checks.

@swcurran
Copy link
Contributor

swcurran commented Jun 9, 2023

I’d say remove the dependency. We don’t want them lightly added. And until you are a maintainer, please just get me to re-kickoff the tests. I can do that.

@andrewwhitehead
Copy link
Contributor

Should we be updating cred_ex_record.thread_id if it's unset?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@usingtechnology
Copy link
Contributor Author

Should we be updating cred_ex_record.thread_id if it's unset?

It was always being set AFTER being used to set thid.

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.

Messages with an empty ~thread decorator
4 participants