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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

zip action did not return output path with zip if not given with one #9159

Merged
merged 1 commit into from May 15, 2017
Merged

zip action did not return output path with zip if not given with one #9159

merged 1 commit into from May 15, 2017

Conversation

joshdholtz
Copy link
Member

zip action now will always return an output path with the .zip extension 馃槉

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

馃摑 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@joshdholtz
Copy link
Member Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

hjanuschka
hjanuschka reviewed May 12, 2017
View reviewed changes
fastlane/lib/fastlane/actions/zip.rb

absolute_output_path = File.expand_path(params[:output_path])

# Appends ".zip" if path does not end in ".zip"
absolute_output_path = absolute_output_path.gsub(/(?<!.zip)$/, ".zip")
Copy link
Collaborator

Choose a reason for hiding this comment

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

i love regex 鉂わ笍

Copy link
Member Author

Choose a reason for hiding this comment

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

An awesome viewer helped me with that one 馃槆

Copy link
Member

Choose a reason for hiding this comment

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

Why do we use a regex here, instead of absolute_output_path.end_with?(".zip") or even File.extname(absolute_output_path)? 馃憤

Copy link
Member Author

Choose a reason for hiding this comment

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

@KrauseFx It was the cleanest/most fun single line approach 馃槆 yolo

hjanuschka
hjanuschka approved these changes May 12, 2017
View reviewed changes
Copy link
Collaborator

@hjanuschka hjanuschka left a comment

Choose a reason for hiding this comment

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

saw you fighting against that on stream (awesome btw) - LGTM 馃殌

KrauseFx
KrauseFx approved these changes May 15, 2017
View reviewed changes
Copy link
Member

@KrauseFx KrauseFx left a comment

Choose a reason for hiding this comment

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

Nice work, just one comment 馃憤

fastlane/lib/fastlane/actions/zip.rb

absolute_output_path = File.expand_path(params[:output_path])

# Appends ".zip" if path does not end in ".zip"
absolute_output_path = absolute_output_path.gsub(/(?<!.zip)$/, ".zip")
Copy link
Member

Choose a reason for hiding this comment

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

Why do we use a regex here, instead of absolute_output_path.end_with?(".zip") or even File.extname(absolute_output_path)? 馃憤

@joshdholtz joshdholtz merged commit 9c10c04 into fastlane:master May 15, 2017
@KrauseFx
Copy link
Member

@joshdholtz can you submit a follow-up PR that removes the regex? It's a lot harder to read for developers 馃憤

@joshdholtz joshdholtz mentioned this pull request May 15, 2017
Merged
@fastlane-bot
Copy link

Hey @joshdholtz 馃憢

Thank you for your contribution to fastlane and congrats on getting this pull request merged 馃帀
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 馃憤
We'll notify you once we shipped a new release with your changes 馃殌

@mpirri mpirri mentioned this pull request May 17, 2017
Merged
@fastlane-bot
Copy link

Congratulations! 馃帀 This was released as part of fastlane 2.31.0 馃殌

@fastlane fastlane locked and limited conversation to collaborators Aug 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KrauseFx KrauseFx KrauseFx approved these changes

@hjanuschka hjanuschka hjanuschka approved these changes

Assignees
No one assigned
Labels
cla: yes status: released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@joshdholtz @googlebot @KrauseFx @fastlane-bot @hjanuschka