-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Don't append a slash to file paths #9713
Don't append a slash to file paths #9713
Conversation
|
||
if (!value.endsWith('/')) { | ||
value += '/'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be addressing a different use case or bug that I am not aware of...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like removing this code is probably fine? Although I can't think of a use case off the top of my head, if someone does need to append a /
they can still do it manually:
cc @AlexSCorey - thoughts?
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -126,6 +126,7 @@ const SCMSubForm = ({ autoPopulateProject, i18n }) => { | |||
}} | |||
aria-label={i18n._(t`Select source path`)} | |||
placeholder={i18n._(t`Select source path`)} | |||
createText={i18n._(t`Set source path to`)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default text shown when manually setting an inventory source is kind of weird:
"Create foo/bar" makes it seem like a file or directory would be created somewhere when it wouldn't be.
When we type in a path like this, we're really just specifying a file location that may or may not exist. Our component library has a parameter for customizing the text so I changed it:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this phrasing
Build succeeded.
|
Build succeeded.
|
note: branch results currently match devel results |
Build succeeded (gate pipeline).
|
for #9701