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

implemented attribute 'download_file_list' #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grumpyshoe
Copy link

Implemented a new attribute 'download_file_list' to specify which file to download from bucket on success

@grumpyshoe
Copy link
Author

Unfortunately I could not create a working test due to my lack of knowledge :( . Could someone else do this and contact me as soon as this has happened? I would like to know how this could be made.

@bffcorreia bffcorreia self-requested a review March 8, 2019 14:54
@bffcorreia bffcorreia added the enhancement 🚀 Improve every day! label Mar 8, 2019
@bffcorreia
Copy link
Contributor

Hello @grumpyshoe!

Thank you for your PR. I will try to give it a look ASAP and create a working test.

Cheers!

@grumpyshoe
Copy link
Author

Hi @bffcorreia,

can you estimate when you will check and take over the PR?

Greetings!

@bffcorreia
Copy link
Contributor

Hopefully until the end of the week 🙂

@MartyCatawiki
Copy link

Wow, this was something i was looking for! I'm now using the fork of @grumpyshoe and that works fantastic 👍
It would be really nice to add this PR to this, any plans to do that @bffcorreia?

Copy link
Contributor

@bffcorreia bffcorreia left a comment

Choose a reason for hiding this comment

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

Hi @grumpyshoe!

Sorry for my very late response. I'm currently very busy. I reviewed your code. Can you just check my comments?

Currently I don't have time to create tests for this feature but since there are people already using your fork, I think we can merge it and I will add the tests in the future.

Cheers and many thanks for your PR.

@@ -160,6 +160,13 @@ run_tests_firebase_testlab(
<td>true</td>
</tr>

<tr>
<td>download_file_list</td>
<td>A list of files that should be downloaded from the bucket or not, seperated by space. This is a additional parameter for 'download_results_from_firebase'</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix typo: "a additional parameter" to "an additional parameter"

FastlaneCore::ConfigItem.new(key: :download_file_list,
env_name: "DOWNLOAD_FILE_LIST",
description: "A list of files that should be downloaded from the bucket or not, seperated by space. This is a additional parameter for 'download_results_from_firebase'. Default: empty string",
is_string: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to be a string? If this parameter is a list, it is probably better to use an Array. Can we change this?

if params[:download_file_list] && !params[:download_file_list].empty?
UI.message("Get files at bucket...")

params[:download_results_from_firebase] = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this?

@bffcorreia
Copy link
Contributor

Hi @MartyCatawiki!

I reviewed the code now. I hope I can merge it soon :)

Cheers

@MartyCatawiki
Copy link

Hi @grumpyshoe i was wondering if you can find some time to take a look at the comments of @bffcorreia. I would be really nice to have this PR added. 😃

@grumpyshoe
Copy link
Author

grumpyshoe commented Aug 15, 2019

HI @bffcorreia @MartyCatawiki ,

sorry for my late response, but I was quite busy too...
I will have a look at your comments und update the PR as soon as possible!

Stay Tuned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🚀 Improve every day!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants