-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Handling Unicode in Project Generation #1345
Conversation
@nirbhay24 Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@nirbhay24 Thank you for signing the Contributor License Agreement! |
@snicoll please comment on changeset |
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.
Thanks for the PR. I've added a couple comments for your consideration. Let me know if you have time.
@@ -66,7 +66,7 @@ public void setDependencies(List<String> dependencies) { | |||
} | |||
|
|||
public String getName() { | |||
return this.name; | |||
return cleanInputValue(this.name); |
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 think it would be more natural to do this in DefaultProjectRequestToDescriptionConverter
, perhaps in a way that makes it available to sub-classes if necessary.
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.
Yes Make sense, To keep original request as it is, and override in changes in DefaultProjectRequestToDescriptionConverter
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.
Updated code for same
@@ -175,4 +175,11 @@ public void setBaseDir(String baseDir) { | |||
this.baseDir = baseDir; | |||
} | |||
|
|||
private String cleanInputValue(String inputString) { | |||
if (StringUtils.hasText(inputString)) { | |||
return org.apache.commons.lang3.StringUtils.stripAccents(inputString); |
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 wonder if we should do the simple thing for now (as described in the article in the issue). We can expand if necessary, rather than calling third-party code.
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.
As it was already in dependency, so thought we can use it. But yes we can have more control if we use over methods.
updated code for same
@snicoll Thanks for comments, I have updated PR |
This commit replaces characters with accents with their US-ascii counterpart. See gh-1345
@nirbhay24 thank you for making your first contribution to Spring Initializr. |
My pleasure 👍 |
Fix for Bug : #1328
There can we two approach to handle this scenario
1- Have Validation on request and give error to caller.
2- We don't give error But clean input request to safe value -like to stripAccents
Demö to be used as Demo
köykkä to be used as koykka
we can add more cleaning options