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

Use ValueTuple instead of Tuple for internal cache #168

Merged
merged 2 commits into from Dec 9, 2018

Conversation

Projects
None yet
6 participants
@sharwell
Member

sharwell commented Dec 5, 2018

No description provided.

@sharwell sharwell requested a review from dotnet/dotnet-winforms as a code owner Dec 5, 2018

// See the LICENSE file in the project root for more information.

using System.Collections.Concurrent;
using System.Runtime.InteropServices;

This comment has been minimized.

@sharwell

sharwell Dec 5, 2018

Member

📝 Not considering the new use of ValueTuple, The only difference between this file and the one that's used is the placement of these using directives.

@zsd4yr

This comment has been minimized.

Member

zsd4yr commented Dec 5, 2018

@sharwell which Issue does this address?

@zsd4yr zsd4yr self-requested a review Dec 5, 2018

@zsd4yr

This comment has been minimized.

Member

zsd4yr commented Dec 5, 2018

Would you mind also writing some unit tests for IsApiAvailable against expected and erroneous values? I believe CommonUnsafeNativeMethods should have some values which are expected to work

@sharwell

This comment has been minimized.

Member

sharwell commented Dec 5, 2018

@sharwell which Issue does this address?

No specific issue. I observed this by accident while investigating something else.

Would you mind also writing some unit tests for IsApiAvailable against expected and erroneous values?

I can try to get these but it may take me a bit to complete the update.

@sharwell

This comment has been minimized.

Member

sharwell commented Dec 5, 2018

@zsd4yr Added tests

@zsd4yr

This comment has been minimized.

Member

zsd4yr commented Dec 7, 2018

@sharwell which Issue does this address?

No specific issue. I observed this by accident while investigating something else.

@sharwell would you mind making one? Simply to track this work independent of the PR

@sharwell

This comment has been minimized.

Member

sharwell commented Dec 7, 2018

would you mind making one? Simply to track this work independent of the PR

For most of the projects I maintain, I apply labels to the pull request itself if and only if it doesn't strictly resolve one or more existing issues. On GitHub, pull requests are treated roughly equivalent to issues for search/triage/reference purposes, so you can apply labels to it to treat it as both a pull request and an issue. An issue could be created, but it would really just be noise and a new level of indirection.

@sharwell

This comment was marked as resolved.

Member

sharwell commented Dec 7, 2018

📝 Please note my added comment above:

⚠️ Please DO NOT rebase or squash this change during the merge. ⚠️

I didn't realize in #172 that this wasn't the default for cross-contributor merges in this repository so I wanted to call out the request explicitly.

@karelz

This comment has been minimized.

Member

karelz commented Dec 7, 2018

@sharwell what is the reason for no squash? Do you want to preserve history of different changes which is valuable in this specific case? Or is it something you believe is right all the time?

I agree we don't need a new issue in this case. It would not bring additional value.
In general, PRs are poor place for discussions - we ask to open new issues when a PR adds new API (it needs API review), or when the solution needs larger design discussion, or discussion if it is worth fixing at all.

BTW: We do not place labels on PRs - see PR guidence.

@zsd4yr

This comment has been minimized.

Member

zsd4yr commented Dec 7, 2018

^ the guidance has changed. I will label the pr appropriate to the request not to squash.

@sharwell sharwell force-pushed the sharwell:use-valuetuple branch from ddb3300 to 63231c7 Dec 7, 2018

@sharwell

This comment has been minimized.

Member

sharwell commented Dec 7, 2018

I'm rebasing this the following commits:

  1. Shared ApiHelper.cs and add tests
  2. Update ApiHelper to use (string, string) instead of Tuple<string, string>

I'll push the second commit after I see the first one pass CI.

@sharwell sharwell force-pushed the sharwell:use-valuetuple branch from 63231c7 to 03a214a Dec 7, 2018

@sharwell sharwell removed the * NO-MERGE * label Dec 7, 2018

@zsd4yr

zsd4yr approved these changes Dec 9, 2018

@zsd4yr zsd4yr merged commit d28ecff into dotnet:master Dec 9, 2018

1 check passed

license/cla All CLA requirements met.
Details

@sharwell sharwell deleted the sharwell:use-valuetuple branch Dec 9, 2018

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