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

Update Calc x:Name values to begin with uppercase #62

Closed
MicrosoftIssueBot opened this issue Feb 27, 2019 · 16 comments · Fixed by #433
Closed

Update Calc x:Name values to begin with uppercase #62

MicrosoftIssueBot opened this issue Feb 27, 2019 · 16 comments · Fixed by #433
Labels
codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) help wanted Issues identified as good community contribution opportunities Pri: 3

Comments

@MicrosoftIssueBot
Copy link
Collaborator

MicrosoftIssueBot commented Feb 27, 2019

Any and all occurrences of x:Name should use Pascal-case. Right now, it is inconsistent between views. The reasoning is that x:Name converts the object to be accessible as a property of the current view and properties should be Pascal-cased. I think this issue is easily fixed with a regex + replace.

@MicrosoftIssueBot
Copy link
Collaborator Author

This is your friendly Microsoft Issue Bot. I created this issue automatically as requested by a team member.

@MicrosoftIssueBot MicrosoftIssueBot added help wanted Issues identified as good community contribution opportunities Pri: 3 and removed Pri: 2 labels Feb 27, 2019
@grochocki grochocki added needs more info Issue requires more information from poster and removed Enhancement labels Feb 28, 2019
@LanceMcCarthy
Copy link
Contributor

Can I get clarification on which views are to be updated?

My understanding of the request is the x:Name and x:Uid of XAML-defined UIElement should use Pascal casing:

Ex.

<TextBlock x:Name="firstNameTextBox" />

Should be

<TextBlock x:Name="FirstNameTextBox" />

@danbelcher-MSFT
Copy link

Any and all occurrences of x:Name should use Pascal-case. Right now, it is inconsistent between views. The reasoning is that x:Name converts the object to be accessible as a property of the current view and properties should be Pascal-cased. I think this issue is easily fixed with a regex + replace.

@danbelcher-MSFT
Copy link

x:Uid I am less concerned with, although following the same convention feels appropriate. This would require updating the values in the resw files. Also accomplishable with Replace.

@LanceMcCarthy
Copy link
Contributor

Okay, I can take this one

PS - I'm avoiding modifying C++ as much as possible if you couldn't tell already :)

@danbelcher-MSFT
Copy link

That's fine, there is a lot of impactful work that can be done in just XAML. I do enjoy discussing C++, however, and I'm happy to recommend learning resources ranging across different levels of experience :)

@grochocki grochocki removed the needs more info Issue requires more information from poster label Mar 2, 2019
@mrlacey
Copy link

mrlacey commented Mar 2, 2019

For scenarios like this, where there's an unwritten rule about naming conventions, I like to create a test (or tests) to ensure any future changes follow the rule too.
How do others feel about this?
Such a test would iterate over all XAML files in the project and fail if any of the defined u:Id or x:Name values do not have the correct casing pattern.
Would such a test be appropriate in the 'CalculatorUnitTests' project or should it go somewhere else?

@grochocki
Copy link
Contributor

IMO, leveraging unit tests to enforce a style/naming convention feels a bit unorthodox. XAML Styler is used to enforce other rules in XAML files in the project; perhaps this is a useful feature for the that tool?

@LanceMcCarthy
Copy link
Contributor

LanceMcCarthy commented Mar 3, 2019

FYI - This cannot be solved with a simple regex and replace all. There are a ton of visual state triggers and style setters that will get erroneously overwritten.

It's a case of one at a time Find and Replace, taking care not to accidentally change AutomationIDs and Uids before those tasks are directly approached.

I'm about 1/2 way through the views now.

If you do know of a regex that works in VS2017 for matching: {x:Name"}{[A-Z\s+]}{[a-z0-9]*} to stop at camel-casing names, that would save me a little time, but not much. It's easy to click past x:Name= matches that are properly cased.

@danbelcher-MSFT
Copy link

Hey Lance, this regex will find x:Names that are camel-cased: x:Name="[a-z] Or were you looking for something different?

@LanceMcCarthy
Copy link
Contributor

That was my first try, but the quotes were messing with the regex-Find, when I removed it, I got no results. I figured it just me messing up the expression.

I'll try it again, but use Resharper for C++.

@danbelcher-MSFT
Copy link

Works on my end:
image
image

@LanceMcCarthy
Copy link
Contributor

LanceMcCarthy commented Mar 10, 2019

It looks like when we went from private to public, something was broken with my private copy of the repo (it was forked a while before this went public). It removed linked commits, preventing me from opening a Pull Request and automatically unassigned me from this task.

To fix this, I had to re-fork the public repo and copy over my fixes to the latest master branch.

Unfortunately, this means all the work is going to be inside a single commit. However, this also allowed me to manually check for conflicts before pushing the commit.

@LanceMcCarthy
Copy link
Contributor

Opened #244

@jlaanstra
Copy link
Contributor

Are we sure we want to change all x:Uid keys? How will the loc system handles this?

@grochocki grochocki added the codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) label Mar 15, 2019
@danbelcher-MSFT
Copy link

@jlaanstra has a good point. It's unfortunate that we wasted Lance's time making that change. @LanceMcCarthy, I apologize for that. We should revert the changes to the resw and focus on only updating the x:Names.

@danbelcher-MSFT danbelcher-MSFT changed the title Update Calc x:Name and x:Uid values to begin with uppercase Update Calc x:Name values to begin with uppercase Mar 24, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) help wanted Issues identified as good community contribution opportunities Pri: 3
Projects
None yet
6 participants