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

Arch values with spaces causes Error #487

Closed
raybooysen opened this issue Sep 9, 2016 · 5 comments
Closed

Arch values with spaces causes Error #487

raybooysen opened this issue Sep 9, 2016 · 5 comments

Comments

@raybooysen
Copy link

With this config:

options = {
  arch: 'x64, ia32'
}

The packager will error as ias32 is not the same as ia32, there is a space after the comma. Happy to commit a PR, to sanitise the options when they're passed in. Is this something the project would accept?

@malept
Copy link
Member

malept commented Sep 9, 2016

Out of curiosity, is there a reason why you're specifying it that way instead of as an Array?

Whether it would get accepted depends on a couple of things:

  • what you mean by "sanitized"
  • you write tests for your changes

@raybooysen
Copy link
Author

I'm specifying it as comma separated because the documentation says it supported it.

Sanitized -> I mean simple string trimming
Tests -> Definitely

@raybooysen
Copy link
Author

Just to be complete, here is the snippet from the docs:

Arbitrary combinations of individual architectures are also supported via a comma-delimited string or array of strings.

@malept
Copy link
Member

malept commented Sep 9, 2016

I think I would rather have it split on the regular expression /, */ than trim each value. But other than that, it sounds fine.

@raybooysen
Copy link
Author

Sure, sounds sensible

@malept malept added the help wanted Needs a contributor from the community label Nov 2, 2016
malept added a commit that referenced this issue Jun 19, 2017
@malept malept added has-pull-request and removed help wanted Needs a contributor from the community labels Jun 20, 2017
@malept malept self-assigned this Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants