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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tests for upload command #124

Merged
merged 6 commits into from
Jan 5, 2021
Merged

Tests for upload command #124

merged 6 commits into from
Jan 5, 2021

Conversation

a5net
Copy link
Contributor

@a5net a5net commented Dec 24, 2020 •
edited
Loading

What was wrong?

Tests for the upload command were missing. Related to #118.

How was it fixed?

This PR implements these tests.

Cute Animal Picture

Cute animal picture

@a5net a5net changed the title implements test for upload command Tests for upload command Dec 24, 2020
@a5net a5net marked this pull request as ready for review December 24, 2020 17:06
pipermerriam
pipermerriam approved these changes Dec 28, 2020
View reviewed changes
tests/forms/test_address_upload.py Outdated Show resolved Hide resolved
tests/forms/test_address_upload.py Show resolved Hide resolved
tests/forms/test_address_upload.py Outdated Show resolved Hide resolved
@a5net
Copy link
Contributor Author

a5net commented Dec 29, 2020

Also, I won't be able to merge this because I am not a contributor in this project.

@ligi
Copy link
Member

ligi commented Jan 2, 2021

I will merge PR's when they are approved by @pipermerriam

@pipermerriam
Copy link
Collaborator

pipermerriam commented Jan 3, 2021 via email

pipermerriam
pipermerriam approved these changes Jan 3, 2021
View reviewed changes
Copy link
Collaborator

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Looks solid. You should have merge permissions now @ayanginet

tests/forms/test_address_upload.py Outdated Show resolved Hide resolved
tests/forms/test_address_upload.py Outdated Show resolved Hide resolved
tests/forms/test_address_upload.py Show resolved Hide resolved
tests/forms/test_address_upload.py Show resolved Hide resolved
tests/forms/test_address_upload.py Outdated Show resolved Hide resolved
@ligi
Copy link
Member

ligi commented Jan 3, 2021

I removed the merge access again. Please let me merge all PRs so I know what is going in the codebase.
@ayanginet I can merge - just give me a signal. Just waiting if you want to address some of the comments of @pipermerriam - I do not think there are blockers in there - still would wait for your ack.
@pipermerriam thanks for the review

pipermerriam
pipermerriam approved these changes Jan 4, 2021
View reviewed changes
Copy link
Collaborator

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

@ligi my standard for reviews is:

  • comment: just a comment about something and not a full review
  • request changes: there is something about the PR that I feel needs to be changed and I would like to review it again once it has been changed.
  • approve: Once the author has addressed my comments to their satisfaction they may merge at their discretion.

If you (@ligi ) want to be the one who merges, then I think our process here will need to be:

  1. ayan/nurtas request review from me
  2. I review until PR is satisfactory for me to mark as "approved"
  3. ayan/nurtas signals to @ligi that they have made all the changes they want to make
  4. @ligi merges or kicks PR back if there is something else that needs to be addressed.

@ligi
Copy link
Member

ligi commented Jan 4, 2021

Sounds good to me - looks like we are at step 3 now ;-)

@a5net
Copy link
Contributor Author

a5net commented Jan 5, 2021

@ligi I think this PR is ready to be merged 👍

@ligi ligi merged commit d22fbde into efdevcon:master Jan 5, 2021
@a5net a5net deleted the upload-test branch January 5, 2021 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pipermerriam pipermerriam pipermerriam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@a5net @ligi @pipermerriam