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

Houston, we have some suggestions #204

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

EverlastingBugstopper
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper commented Jan 26, 2021

this PR builds off of the foundation in #186 and adds some suggestions for HoustonProblem.

@EverlastingBugstopper EverlastingBugstopper added feature 🎉 new commands, flags, functionality, and improved error messages status - needs review labels Jan 26, 2021
@EverlastingBugstopper EverlastingBugstopper added this to the 🐣 0.1.0 milestone Jan 26, 2021
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch from e5bc1ab to 2444a68 Compare January 26, 2021 18:36
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch 2 times, most recently from 2406990 to 540ab20 Compare January 26, 2021 19:36
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch 5 times, most recently from 57bbfea to d001533 Compare January 26, 2021 20:44
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch 4 times, most recently from 4e0e626 to 4ef3dd2 Compare January 26, 2021 21:32
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch from 4ef3dd2 to 4addcb3 Compare January 26, 2021 21:51
Copy link
Member

@lrlna lrlna left a comment

Choose a reason for hiding this comment

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

If the mountain will not come to Mohammed, then Mohammed must go to the mountain

In regards to the env problem: what if instead of moving env to its own crate, we move rover-error to rover? I don't see us publishing either of these as crates in the near future, as I see them as application level code. But if something changes, we can always reconsider and factor both env and rover-error out.

What do you think?

@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/errorerrorerrorerror branch 2 times, most recently from 9b387ba to 7bf1b34 Compare January 27, 2021 17:46
@EverlastingBugstopper
Copy link
Contributor Author

yup, that makes sense to me. i've updated #186 to make error a module in rover instead of its own crate 😄

@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch from 4addcb3 to 360d94c Compare January 27, 2021 18:00
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch 4 times, most recently from 9b44db7 to cc10817 Compare January 27, 2021 18:49
@EverlastingBugstopper
Copy link
Contributor Author

Mohammed has gone to the mountain

@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch 2 times, most recently from ea84677 to 5e32d77 Compare January 27, 2021 18:55
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch from 5e32d77 to 9a2dc6c Compare January 27, 2021 19:55
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch from 9a2dc6c to 728881d Compare January 27, 2021 20:02
Base automatically changed from avery/errorerrorerrorerror to main January 27, 2021 20:08
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch from 728881d to 3ec9919 Compare January 27, 2021 20:08
@EverlastingBugstopper
Copy link
Contributor Author

Alright - this is ready for re-review. I've addressed the requested changes but before merging this, I'd like an explicit approval from @lrlna given they marked this as "Changes Requested" 😄

Copy link
Member

@lrlna lrlna left a comment

Choose a reason for hiding this comment

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

looks really good!

@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/houston-we-have-some-suggestions branch from 3ec9919 to 0995efc Compare January 28, 2021 15:40
@EverlastingBugstopper EverlastingBugstopper merged commit 17fd355 into main Jan 28, 2021
@EverlastingBugstopper EverlastingBugstopper deleted the avery/houston-we-have-some-suggestions branch January 28, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants