Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

Use record to get with keyword. #1040

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

cgranade
Copy link
Contributor

This PR modifies the SubmissionOptions class to be a record class, allowing other code to use the with keyword to modify instances of the class on a field-by-field basis. The existing With method can then be refactored to use the with keyword.

@@ -60,7 +65,12 @@ public class SubmissionOptions
int? shots = null,
ImmutableDictionary<string, string>? inputParams = null,
string? targetCapability = null) =>
new SubmissionOptions(
friendlyName ?? FriendlyName, shots ?? Shots, inputParams ?? InputParams, targetCapability ?? TargetCapability);
this with
Copy link
Contributor

@bamarsha bamarsha Jun 14, 2022

Choose a reason for hiding this comment

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

I'd rather remove With, since it's redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, my only thought was to not break existing callers, but happy to go on and pull that method, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To unblock e2e, would it be OK to mark this and Default as obsolete for now?

namespace Microsoft.Quantum.Runtime.Submitters
{
/// <summary>
/// Options for a job submitted to Azure Quantum.
/// </summary>
public class SubmissionOptions
public record SubmissionOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be better to remove Default and the private constructor, and instead give each init property a default value?

@@ -5,32 +5,37 @@

#nullable enable

namespace System.Runtime.CompilerServices
{
public class IsExternalInit {}
Copy link
Contributor

Choose a reason for hiding this comment

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

As a public type, won't this conflict if another reference uses the same workaround? Can we update Microsoft.Quantum.Runtime.Core target framework to net6.0 instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested in other places, and that type seems to be special-cased to not conflict, allowing this as a suggested workaround (as per https://developercommunity.visualstudio.com/t/error-cs0518-predefined-type-systemruntimecompiler/1244809).

@AtariDreams
Copy link

Any updates on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants