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

Simplify encoded value generation fixing lifetime issue #1000

Merged
merged 2 commits into from
Aug 11, 2022
Merged

Simplify encoded value generation fixing lifetime issue #1000

merged 2 commits into from
Aug 11, 2022

Conversation

satlank
Copy link
Contributor

@satlank satlank commented Aug 11, 2022

Shadowing encoded makes the async block not send which prevents
sharing it across threads. Just do the generation of the encoded values
through temporary variables instead of shadowing.

The added code in the test module will fail to compile without the
accompanying code changes.

Shadowing `encoded` makes the async block not send which prevents
sharing it across threads.  Just do the generation of the encoded values
through temporary variables instead of shadowing.

The added code in the test module will fail to compile without the
accompanying code changes.
@ghost
Copy link

ghost commented Aug 11, 2022

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Great catch! This code is also more readable too.

I'd like the test to be simpler though (and I offer a way that could look), and we should also make sure the test is clear what is being tested.

sdk/identity/src/device_code_flow/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

🎉

@rylev rylev merged commit 7133803 into Azure:main Aug 11, 2022
@satlank satlank deleted the fixDeviceCodeFlowLifetime branch August 11, 2022 15:47
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.

2 participants