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

Dyn 4715 package name special characters #12802

Merged
merged 29 commits into from
Apr 20, 2022
Merged

Dyn 4715 package name special characters #12802

merged 29 commits into from
Apr 20, 2022

Conversation

jesusalvino
Copy link
Contributor

@jesusalvino jesusalvino commented Apr 14, 2022

Purpose

Prevent user from enter some special characters as part of the package name in the Publish Package feature.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

###Reviewers
@QilongTang

###FYIs
@RobertGlobant20 @filipeotero

@jesusalvino
Copy link
Contributor Author

SpecialCharacters

@@ -723,6 +757,94 @@
</Setter>
</Style>

<Style x:Key="InputStyleWithIcon" TargetType="{x:Type TextBox}">
Copy link
Contributor

@QilongTang QilongTang Apr 14, 2022

Choose a reason for hiding this comment

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

What are the changes to this style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are the changes to this style?

adds an extra space to the right side of the input to draw the Icon that will be displayed if required

@QilongTang
Copy link
Contributor

QilongTang commented Apr 18, 2022

Regression also to look at

[Test Result] (1 failure / +1)
[Dynamo.Tests.Search.SearchDictionaryTest.TestContainsSpecialCharacters]

@QilongTang
Copy link
Contributor

One comment then LGTM

@QilongTang QilongTang added this to the 2.15.0 milestone Apr 19, 2022
@zeusongit
Copy link
Contributor

So just to be clear this will not affect the packages which already have spaces and want to publish a new version, but will prohibit future package names to include spaces, right? @QilongTang @jesusalvino
I think we should also make a special note of it in the next release.

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 19, 2022

So just to be clear this will not affect the packages which already have spaces and want to publish a new version, but will prohibit future package names to include spaces, right? @QilongTang @jesusalvino I think we should also make a special note of it in the next release.

What about users publishing new versions of existing packages with spaces? Won't this stop them from updating existing packages? - same question as @zeusongit 👍

{
return element.Contains("*") || element.Contains(".") || element.Contains(" ")
|| element.Contains("\\");
// Excluding white spaces and uncommon characters, only keeping the displayed in the Windows alert
Copy link
Contributor

@QilongTang QilongTang Apr 19, 2022

Choose a reason for hiding this comment

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

FYI.. @zeusongit @mjkkirschner see this comment, space is excluded from .NET API so not considered special char in Dynamo

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification @QilongTang 👍🏼

@QilongTang
Copy link
Contributor

So just to be clear this will not affect the packages which already have spaces and want to publish a new version, but will prohibit future package names to include spaces, right? @QilongTang @jesusalvino I think we should also make a special note of it in the next release.

What about users publishing new versions of existing packages with spaces? Won't this stop them from updating existing packages? - same question as @zeusongit 👍

See my comment in code

@QilongTang
Copy link
Contributor

@jesusalvino Looks like there are still regressions to deal with before we can merge

{
if (invalidCharacters == null)
{
invalidCharacters = Search.SearchDictionary<object>.SpecialAndInvalidCharacters();
Copy link
Contributor

@QilongTang QilongTang Apr 20, 2022

Choose a reason for hiding this comment

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

Looking at this PR again, I still would not prefer this way. The original getter of invalid chars lives inside of search dictionary which seems strange. Can you move this static getter to https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoUtilities/PathHelper.cs and have ContainsSpecialCharacters refer to this property? And instead of the getter pointing to a function elsewhere, I would just return System.IO.Path.GetInvalidFileNameChars().Where(x => !char.IsWhiteSpace(x) && (int)x > 31).ToArray();

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

More changes requested @jesusalvino

@QilongTang QilongTang merged commit a496720 into DynamoDS:master Apr 20, 2022
@jesusalvino jesusalvino deleted the DYN-4715-Package-Name-Special-Characters branch May 20, 2022 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants