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

grooming code #484

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

amites
Copy link
Contributor

@amites amites commented Jul 6, 2021

This PR closes n/a

What does this PR do?

Grooming code structure while creating docs

How does this PR make you feel? 🔗

![good]

@netlify
Copy link

netlify bot commented Jul 6, 2021

👷 Deploy request for upswyng pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: 3eae401

@amites amites marked this pull request as ready for review July 14, 2021 03:54
Copy link
Member

@jollyjerr jollyjerr left a comment

Choose a reason for hiding this comment

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

Screen Shot 2021-07-16 at 7 47 34 PM

Looks like an import got messed up somewhere. This is also a lot to test - what if we break apart the "small" stuff (typos and stuff) from the bigger stuff like adding awaits and deleting larger chunks of code. We want to make sure that everything behaves exactly as it is - and we don't really have a lot of resources to do a full regression test right now 😅. I love what is going on in here but I think taking it in smaller steps might be best? Jacob may disagree though and that's totally fair - I'm good with anything!

@amites
Copy link
Contributor Author

amites commented Jul 22, 2021

removed all the logic changes adding await commands and left all the pure "grooming"

added a bunch of comments to help simplify review -- let me know if ya need to break this down further

@jollyjerr jollyjerr dismissed their stale review July 28, 2021 15:45

Will take another look as soon as I get a chance!

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

Successfully merging this pull request may close these issues.

2 participants