Skip to content

Add NullableHelper #3389

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.

Sign up for GitHub

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

Jump to bottom
Merged
5 commits merged into from
Mar 4, 2021
Merged

Add NullableHelper #3389

5 commits merged into from
Mar 4, 2021

Conversation

john-h-k
Copy link
Contributor

cc @Sergio0694

Introduce a NullableHelper type with GetValueOrDefaultRef which returns a ref T which points to a value that is equivalent to GetValueOrDefault

TODO add tests

@ghost
Copy link

ghost commented Jul 17, 2020

Thanks john-h-k for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost assigned Kyaa-dost Jul 17, 2020
Sergio0694
Sergio0694 previously requested changes Jul 18, 2020
View reviewed changes
Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Looks like an interesting idea! 😊

Needs some work though, I've left some review comments and we can start from there!

@john-h-k john-h-k marked this pull request as draft July 19, 2020 11:53
@ghost
Copy link

ghost commented Jul 19, 2020

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@Sergio0694 Sergio0694 added feature 💡 high-performance 🚂 Issues/PRs for the Microsoft.Toolkit.HighPerformance package needs author feedback 📝 and removed needs attention 👋 labels Jul 19, 2020
@Sergio0694
Copy link
Member

@azchohfi Not sure who handles the bot, but what happened seems like a bug?

I have a series of requested changes still pending, @john-h-k made the PR a draft and the bot removed the "Needs author feedback" label and added "Needs attention" despite the fact that the PR is now a draft and that all those requested changes are still there and not addressed. Should we report this or open an issue for the bot somewhere? 🤔

I mean:

image

@Kyaa-dost
Copy link
Contributor

Kyaa-dost commented Jul 21, 2020 •
edited
Loading

@Sergio0694 I handle the bot 😄 The two labels "needs author feedback" (for author) and "needs attention" (for the team and community) are being utilized as a communication source here. The whole idea of these two labels was that any communication from the team and community will trigger the label "needs author feedback" and any communication from the author should place "needs attention".

In this scenario, after all the comments you made, it triggered "needs author feedback." Then because "john-h-k marked this pull request as draft 2 days ago" count as an action therefore bot triggered the "needs attention" label for the rest of the team. There are some situations where the bot is not smart enough to understand the communication because of the certain actions that have been set. I can explore more into this. Thanks 😄

image

@Sergio0694
Copy link
Member

Sergio0694 commented Jul 21, 2020 •
edited
Loading

Hey @Kyaa-dost, thank you for chiming in and for the explanation! 😊
I didn't know you handled the bot (or I would've tagged you directly), that's good to know for the future!

Yeah the "needs attention" and "needs author feedback" tags are indeed very useful to notify about changes as you said, that was a very good idea! I was just wondering whether it would make sense to explore a specific set of actions (eg. such as just changing a PR from ready to draft) to mark as not counting as triggers for the bot, so that the tags would only be applied upon "meaningful" actions on behalf of users (eg. actual commits, or replies to comments, etc.). Though I understand that this could also get pretty tricky (what actions specifically to choose, where to draw the line exactly, etc.).

Just some food for thought in general, I really do like how the bot works already though, great job on that! 😄

@Kyaa-dost
Copy link
Contributor

@Sergio0694 Correct, the whole idea of these labels is effective communication between the author and the team. We just want to make sure both the parties are aware of any changes in the PR whether it's commits, comments, etc. I can certainly look more into different options like Drafts and explore the possibilities.

Thanks for the feedback and always feel free to let me know any changes or feature you would like to ease the workflow 😄

@ghost ghost added the no-recent-activity 📉 Open Issues that require attention label Aug 7, 2020
@ghost
Copy link

ghost commented Aug 7, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 30 days of this comment.

@michael-hawker
Copy link
Member

@john-h-k just wanted to check-in and see if you wanted to take a look at this more and address the feedback from @Sergio0694? If you're busy, that's fine, just give us a heads up and ETA when you think you might get back to it so we can circle-back later at that time. Thanks!

@ghost ghost removed the no-recent-activity 📉 Open Issues that require attention label Aug 18, 2020
@ghost ghost added the no-recent-activity 📉 Open Issues that require attention label Sep 3, 2020
@ghost
Copy link

ghost commented Sep 3, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 30 days of this comment.

@Kyaa-dost
Copy link
Contributor

@john-h-k any update on this?

@ghost ghost removed the no-recent-activity 📉 Open Issues that require attention label Sep 25, 2020
@ghost ghost added the no-recent-activity 📉 Open Issues that require attention label Oct 10, 2020
@ghost
Copy link

ghost commented Oct 10, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 30 days of this comment.

@Kyaa-dost
Copy link
Contributor

@john-h-k have a chance to look at the suggestions @Sergio0694 provided? If not can you please let us know the ET so we can proceed towards a feasible turnaround time. Thanks 😊

@ghost ghost removed the no-recent-activity 📉 Open Issues that require attention label Nov 11, 2020
@ghost ghost added the no-recent-activity 📉 Open Issues that require attention label Nov 26, 2020
@ghost
Copy link

ghost commented Nov 26, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 30 days of this comment.

@ghost
Copy link

ghost commented Jan 29, 2021

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 30 days of this comment.

@michael-hawker
Copy link
Member

@Sergio0694 have you heard from John lately, is this something you wanted to help finish cross over the finish line or should we move to 7.1?

@ghost ghost removed the no-recent-activity 📉 Open Issues that require attention label Feb 18, 2021
@Sergio0694
Copy link
Member

@michael-hawker He's been busy with school and other projects lately, I don't think he'll be able to pick this up again any time soon. I can create a PR based on top of his and finish the feature for 7.0 if you want, sure 😄

@Sergio0694 Sergio0694 force-pushed the master branch 2 times, most recently from 273a32a to f2eb52c Compare February 18, 2021 22:17
@Sergio0694 Sergio0694 marked this pull request as ready for review February 18, 2021 22:17
@Sergio0694
Copy link
Member

I rebased the PR and did some more tweaks myself since John has been busy, should be good for review now.
There's a couple caveats with this API, but other than that it seems ok. I restricted it to .NET 5 to ensure consumers will actually use the layout we expect, to avoid crashes. It's still technically not guaranteed to always be correct (eg. .NET 8, 9, 10, etc. could theoretically update the layout in the future and someone could run an old version of the lib there, which could break, but that's highly unlikely to happen I'd say), but then again the API is called Dangerous, so there's that too 😄

FYI @michael-hawker

@michael-hawker
Copy link
Member

@Sergio0694 thanks, seems straight-forward enough. Is this another thing we'd want as a Helper pattern vs. Extension on every generic nullable?

@michael-hawker michael-hawker added this to the 7.0 milestone Mar 1, 2021
@Sergio0694
Copy link
Member

@michael-hawker I'm thinking leaving this as an extension should be fine, as it's just one method and we already have the same for things such as array extensions and whatnot, plus the method itself is restricted to nullable value types. The main issue with the object extensions is that they were extremely niche (and dangerous), and always visible because almost everything can be cast to an object, so those were just always crowding up IntelliSense. Don't think it'd be worth it to have a static helper with just this 🙂

@michael-hawker
Copy link
Member

@Sergio0694 @RosarioPulella if one of you want to sign-off we can merge this in!

@ghost ghost added the in progress 🚧 label Mar 2, 2021
@ghost
Copy link

ghost commented Mar 2, 2021

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Kyaa-dost
Kyaa-dost approved these changes Mar 3, 2021
View reviewed changes
Copy link
Contributor

@Kyaa-dost Kyaa-dost left a comment

Choose a reason for hiding this comment

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

This is ready to 🚀 🚀

@michael-hawker
Copy link
Member

@Kyaa-dost we can merge this one too once the other two in our merge pipeline go forward... 🙂

Maybe we need to write our own bot to help the other bot??? 😋

@ghost ghost merged commit 4dc90dc into CommunityToolkit:master Mar 4, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rosuavio Rosuavio Rosuavio approved these changes

@michael-hawker michael-hawker michael-hawker approved these changes

@Kyaa-dost Kyaa-dost Kyaa-dost approved these changes

@Sergio0694 Sergio0694 Sergio0694 left review comments

Assignees

@Kyaa-dost Kyaa-dost

Labels
auto merge ⚡ feature 💡 high-performance 🚂 Issues/PRs for the Microsoft.Toolkit.HighPerformance package in progress 🚧
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@john-h-k @Sergio0694 @Kyaa-dost @michael-hawker @Rosuavio