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

Clarification on #142 #160

Closed
cuducos opened this issue Jul 13, 2021 · 2 comments
Closed

Clarification on #142 #160

cuducos opened this issue Jul 13, 2021 · 2 comments

Comments

@cuducos
Copy link
Contributor

cuducos commented Jul 13, 2021

PR #142 is a bit awkward to me. The description has no context or purpose for the changes, which break the tests:

Surely I can fix that, but before I would like to know more about the context of these changes.

My point is: as tests weren't updated, code edits were not mentioned or justified, and no one in the PR discussion seemed to worry about the CI being red I am not sure if this new state is intentional. Therefore I don't know the direction the fix should go:

  • Should the tests be updated to match the code state after the PR being merged?
  • Or should the code edited in the PR be reverted (keeping only the docs update)?

cc people involved in that PR @adorilson @dehatanes @BrunaNayara @julianyraiol @sergiomario
also cc @giuliocc

@adorilson
Copy link

Hi, @cuducos .

The purpose was just to update the README to make easier e clearer the setup process. The only code change was made because we understanding it was in this context too. All changes was intended.

We only done the manual test, and it worked.
The test was not updated just for my fail. They must updated, too.

Thanks for review and fixes in #161 . They was really need.

@cuducos
Copy link
Contributor Author

cuducos commented Jul 15, 2021

All good then 🎉
Great teamwork 💜

@cuducos cuducos closed this as completed Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants