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

Add unique prefixes to all .targets properties and items #321

Merged
merged 2 commits into from
Jun 22, 2022

Conversation

Sergio0694
Copy link
Member

This PR adds unique prefixes to all properties and items in the .targets file bundled with the MVVM Toolkit.
This ensures there can be no conflicts with other targets defining the same elements from other NuGet packages.

Essentially, this PR fixes this: https://github.com/Sergio0694/ComputeSharp/actions/runs/2523630796.

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • Tested code with current supported SDKs
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes
  • Every new API (including internal ones) has full XML docs
  • Code follows all style conventions

@Sergio0694 Sergio0694 added nuget 📦 Changes or issues related to NuGet publishing mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit bugfix 🔧 PRs fixing a discovered bug labels Jun 21, 2022
Copy link
Contributor

@Nirmal4G Nirmal4G left a comment

Choose a reason for hiding this comment

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

Seems like these properties are not meant to be public. So, you can use a _ prefix to denote that. But even _CurrentCompilerVersion sometimes could be used elsewhere as the name is too generic. Hence, my suggestions!

Either _ or _MVVMToolkit_ is fine. But if you think they don't need to be unique then select the former.

@Sergio0694
Copy link
Member Author

"Seems like these properties are not meant to be public. So, you can use a _ prefix to denote that."

@Nirmal4G Is there some official docs or something to document this? I can't find anything regarding the "_" prefix being used specifically for private properties in .targets files or similar. I don't have anything against using that once we confirm this 🙂

@Nirmal4G
Copy link
Contributor

There's no official docs on this as far as I know. It's just a convention (doesn't actually make your property internal/private) to make sure we don't accidentally use potential MSBuild properties that could be used by users.

You can refer to other props and targets shipped by M$FT and you can see this pattern even in Common props and targets.

@Sergio0694
Copy link
Member Author

I see. In that case I don't think we need that, just using "MVVMToolkit" as prefix should be enough. This has been the original suggestion from Sam as well (from the Roslyn team), and what he's been doing as well in the Antlr4cs project too.

@Nirmal4G
Copy link
Contributor

Okay, he doesn't seem to use the prefix even for properties that shouldn't be available to user. I'm okay with that. It looks neat. But you do have targets name prefixed. And it's inconsistent with elsewhere, not that I'm complaining but yeah...😅

@Sergio0694
Copy link
Member Author

Sergio0694 commented Jun 22, 2022

That's a fair point, I think Sam must've also changed his mind on this as he was the one that used "_" in the first place.
I've updated the file in 86aaa99 and removed all "_" prefixes, now we just use "MVVMToolkit___" everywhere 🙂

Copy link
Contributor

@Nirmal4G Nirmal4G left a comment

Choose a reason for hiding this comment

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

Nice... Looks good!

@Sergio0694 Sergio0694 merged commit 90f1db2 into main Jun 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the dev/fix-targets-conflicts branch June 22, 2022 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix 🔧 PRs fixing a discovered bug mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit nuget 📦 Changes or issues related to NuGet publishing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants