Skip to content

Remove dashes from name #260

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.

Sign up for GitHub

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

Jump to bottom
Merged
malept merged 3 commits into from
Mar 12, 2019
Merged

Remove dashes from name #260

malept merged 3 commits into from
Mar 12, 2019

Conversation

@fcastilloec
Copy link
Collaborator

@fcastilloec fcastilloec commented Mar 12, 2019

Fixes #249

@fcastilloec fcastilloec requested a review from malept March 12, 2019 19:00
malept
malept suggested changes Mar 12, 2019
View reviewed changes
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Perhaps use sanitizeName instead?

@fcastilloec
Copy link
Collaborator Author

fcastilloec commented Mar 12, 2019

I thought about it originally, but because dashes do work on Linux for this module, I thought that throwing an error rather than replacing the character might be more appropriate.
I guess we can enforce no-dashes for all OSs rather than have the option of having dashes in some OS and no dashes for others. Thoughts?

@malept
Copy link
Member

malept commented Mar 12, 2019

It's probably better to be consistent rather than have to explain to people that it behaves differently on different host OSes.

@fcastilloec
Copy link
Collaborator Author

fcastilloec commented Mar 12, 2019 •
edited
Loading

I've changed to using sanitizeName and remove the test since it's function it's already tested in the electron-installer-common module

@malept malept changed the title Test for dashes Remove dashes from name Mar 12, 2019
@malept malept merged commit 63f7b1e into master Mar 12, 2019
@malept malept deleted the test-for-dashes branch March 12, 2019 23:17
@fcastilloec fcastilloec mentioned this pull request May 11, 2019
Closed
@fcastilloec fcastilloec mentioned this pull request Jun 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@malept malept malept approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fcastilloec @malept