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.

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

Replace some of our macros to reduce confusion, increase success #9376

Merged
merged 6 commits into from
Mar 4, 2021
Merged

Replace some of our macros to reduce confusion, increase success #9376

merged 6 commits into from
Mar 4, 2021

Conversation

zadjii-msft
Copy link
Member

As mentioned in #9354 (comment)

GETSET_SETTING is too visually similar to GETSET_PROPERTY, but with a VERY different meaning. I think that merely changing the name of the macro would make it harder for us to make this mistake again.

@zadjii-msft zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Mar 4, 2021
@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Mar 4, 2021
@ghost ghost requested review from miniksa, DHowett, leonMSFT and PankajBhojwani March 4, 2021 17:10
@DHowett
Copy link
Member

DHowett commented Mar 4, 2021

I'd rather we keep our macros namespaced to the area they're associated with. SETTING is a touch too generic for me... perhaps INHERITABLE_SETTING which really does explain what it is (rather than GETSET :P)

DHowett
DHowett requested changes Mar 4, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

discussion

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 4, 2021
@DHowett
Copy link
Member

DHowett commented Mar 4, 2021

In fact, I'd also eventually like to rename GETSET_PROPERTY to something like WINRT_MUTABLE_PROPERTY or the like -- not because i don't like getset, but because it is a macro from our winrt helpers.

@zadjii-msft
Copy link
Member Author

Meh sure. Get ready for this PR to touch every single header

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 4, 2021
DHowett
DHowett approved these changes Mar 4, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

nit: update PR title!

@zadjii-msft zadjii-msft changed the title Replace GETSET_SETTING with SETTING Replace some of our macro names to reduce confusion and increase success Mar 4, 2021
@DHowett
Copy link
Member

DHowett commented Mar 4, 2021

btw: if you're doing actual s// you can use \b as part of the regex to match "word boundary". "\bSETTING" means "SETTING but only if it didn't have letters before it" 😄

@DHowett DHowett changed the title Replace some of our macro names to reduce confusion and increase success Replace some of our macros to reduce confusion, increase success Mar 4, 2021
@DHowett DHowett merged commit 3cf7677 into main Mar 4, 2021
@DHowett DHowett deleted the dev/migrie/b/SETTING branch March 4, 2021 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@miniksa miniksa Awaiting requested review from miniksa

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Needs-Second It's a PR that needs another sign-off
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zadjii-msft @DHowett @carlos-zamora