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

Removed any comment that had any form of "To be supplied" #155

Merged
merged 4 commits into from Dec 7, 2018

Conversation

Projects
None yet
6 participants
@carlowahlstedt
Contributor

carlowahlstedt commented Dec 5, 2018

Fixes #50.

Simply removed all of the comments that had any form of "To be supplied" in them.

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

@dnfclas

This comment has been minimized.

dnfclas commented Dec 5, 2018

CLA assistant check
All CLA requirements met.

@Tanya-Solyanik

This comment has been minimized.

Member

Tanya-Solyanik commented Dec 5, 2018

@carlowahlstedt Thank you for picking up an issue! Could you please share regular expression that you used to generate these changes. Change of this size is hard to review.
Also I noticed that you replaced an apostrophe in possessive nouns with a different character, it might be better to re-phrase such comments with no apostrophe.

@carlowahlstedt

This comment has been minimized.

Contributor

carlowahlstedt commented Dec 5, 2018

Didn't use a RegEx, was trying not to have 100 problems 😆. Honestly I'm not that good with them. I used VS Code to do a multiline search and then replaced in bulk.

Not sure what happened with that apostrophe. I'm suspecting an encoding issue. I made the change on a Mac. When I removed it just now, it's still showing something different in the editor. I'll have to dig deeper.

@carlowahlstedt

This comment has been minimized.

Contributor

carlowahlstedt commented Dec 5, 2018

Cloned the original repo, looked at the file and it translated that way by default. Please, let me know what you'd like to do in that scenario.

Is there anything else I can do to help with the size of the changes? I was just trying to do everything from that issue at once.

Removed a stray character
Can I blame the keys everyone hates on the new Macbooks?
@zsd4yr

This comment has been minimized.

Member

zsd4yr commented Dec 5, 2018

Would is not be better to actually supply these?

@zsd4yr

This comment has been minimized.

Member

zsd4yr commented Dec 5, 2018

Didn't use a RegEx, was trying not to have 100 problems 😆. Honestly I'm not that good with them. I used VS Code to do a multiline search and then replaced in bulk.

Not sure what happened with that apostrophe. I'm suspecting an encoding issue. I made the change on a Mac. When I removed it just now, it's still showing something different in the editor. I'll have to dig deeper.

I made a change on my PC and had the same encoding issue @carlowahlstedt --it happens to the best of us

@carlowahlstedt

This comment has been minimized.

Contributor

carlowahlstedt commented Dec 6, 2018

I fixed the two specific instances with the apostrophe mentioned above but there are other instances. I didn't yet dig into why it's doing what it's doing, but might be worth opening an issue to look into it?

screen shot 2018-12-06 at 9 56 24 am

@Tanya-Solyanik

This comment has been minimized.

Member

Tanya-Solyanik commented Dec 6, 2018

I fixed the two specific instances with the apostrophe mentioned above but there are other instances. I didn't yet dig into why it's doing what it's doing, but might be worth opening an issue to look into it?

Makes sense, please open a new issue for a thorough clean up. Thank you!

@Tanya-Solyanik

This comment has been minimized.

Member

Tanya-Solyanik commented Dec 6, 2018

@zsd4yr:

Would is not be better to actually supply these?

Sure, we can do it as we work with individual files more closely, for example when making changes in a public function, we can also improve the comments.

@Tanya-Solyanik

Thank you! looks good.

@merriemcgaw merriemcgaw merged commit 408a31d into dotnet:master Dec 7, 2018

1 check passed

license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment