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

Make 'ID' optional in a WorkItem attribute #67216

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi
Copy link
Member Author

@jcouv ptal.

@jaredpar
Copy link
Member

jaredpar commented Mar 7, 2023

Way back in the days of yore, the URLs for all the various work item types were not so easily parsable. This is why the ID field was added, to let analysis happen on the code base without having to encode 5-10 ways of parsing tricky URLs to find the bug number in question. That was back in the days of yore though, these days everything is easily parsable.

/// </summary>
/// <param name="issueUri">The URI where the work item can be viewed. This is a link to work item in the
/// original source.</param>
public WorkItemAttribute(string issueUri) : this(-1, issueUri)
Copy link
Member

Choose a reason for hiding this comment

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

Why even keep around the other ctor?

Copy link
Member

Choose a reason for hiding this comment

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

Oh did we turn off your other PR to just remove all the instances where we passed the ID?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It was rejected :)

@CyrusNajmabadi
Copy link
Member Author

Why even keep around the other ctor?

One reason si that we have a handful of WorkItems where the id doesn't match between the number and the string. And there are a small handful where the uri is just something like "DevDiv" :)

I'd love to go clean all those up as well, but that's a more onerous task, hunting down what those all mean. For now though, this allows the 99.9% case to migrate. My intent is to change teh IDE to use this universally. Compiler will stay with the existing redundant form for now.

@CyrusNajmabadi CyrusNajmabadi requested a review from jaredpar March 7, 2023 17:33
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1)

@AlekseyTs
Copy link
Contributor

Just a reminder, test only changes can be merged with a single sign-off from the Compiler team.

@CyrusNajmabadi
Copy link
Member Author

Just a reminder, test only changes can be merged with a single sign-off from the Compiler team.

Sounds good. Thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit c977cf7 into dotnet:main Mar 7, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the workItems3 branch March 7, 2023 18:55
@ghost ghost added this to the Next milestone Mar 7, 2023
@jaredpar
Copy link
Member

jaredpar commented Mar 7, 2023

@CyrusNajmabadi

One reason si that we have a handful of WorkItems where the id doesn't match between the number and the string. And there are a small handful where the uri is just something like "DevDiv" :)

Once you merge the other change I'll go through and clean up the rest. The thought being that if I don't have context for whath the right number is probably no one does so can fix or delete.

@CyrusNajmabadi
Copy link
Member Author

The thought being that if I don't have context for whath the right number is probably no one does so can fix or delete.

I def think it can be figured out. For example, if we have WorkItem(1, "...2...") just go look in the db for both of those and see which applies :)

But i def appreciate any help here to make this hte consistent pattern we use for everything. it will help make things much cleaner and simpler (and will avoid this exact issue which clearly happened to a bunch of tests :)).

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

Successfully merging this pull request may close these issues.

4 participants