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

Clean files in local_modules #131

Merged
merged 36 commits into from
Mar 13, 2024
Merged

Clean files in local_modules #131

merged 36 commits into from
Mar 13, 2024

Conversation

kmilo9999
Copy link
Collaborator

  • Cleaning and formatting files in local_modules folder

Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

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

Hi Camilo! I've a couple of things I'd reintroduce (units on physical values) but otherwise this looks good!

@kmilo9999 kmilo9999 requested a review from hollandjg February 26, 2024 20:12
@kmilo9999 kmilo9999 requested a review from cpaniaguam February 27, 2024 16:34
@kmilo9999 kmilo9999 self-assigned this Feb 27, 2024
Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

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

One final set of changes – otherwise looks good!

@kmilo9999 kmilo9999 requested a review from hollandjg February 27, 2024 21:42
hollandjg
hollandjg previously approved these changes Feb 28, 2024
Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

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

Awesome! 🚀

Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

I meant to send this yesterday. Consider seeing if there's anything relevant left!

hollandjg
hollandjg previously approved these changes Mar 1, 2024
Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@kmilo9999
Copy link
Collaborator Author

all good here @cpaniaguam ?

cpaniaguam
cpaniaguam previously approved these changes Mar 1, 2024
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

I am approving but added a bunch of extra suggestions.

Comment on lines 42 to 43
a = "%2.1e" % low
b = "%2.1e" % high
Copy link
Member

Choose a reason for hiding this comment

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

Old style formatting.

Suggested change
a = "%2.1e" % low
b = "%2.1e" % high
a = f"{low:.1e}"
b = f"{high:.1e}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

file re-formatted

Copy link
Member

Choose a reason for hiding this comment

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

This change didn't get applied?


return a[0:3] +'-'+ b +' Hz'
return a[0:3] + "-" + b + " Hz"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return a[0:3] + "-" + b + " Hz"
return f"{a[0:3]}-{b} Hz"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

file re-formatted

@kmilo9999
Copy link
Collaborator Author

@cpaniaguam I finished your last review. Check it again and let me know if I there anything I missed

Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Just a couple more easy changes.

@kmilo9999 kmilo9999 requested a review from cpaniaguam March 13, 2024 15:58
@kmilo9999
Copy link
Collaborator Author

@cpaniaguam Done! Check again.

Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Looks good!

I am not really sure why most of the conversations disappeared. They didn't appear in context so I had to have two windows opened, one at the conversations page and another with the file being reviewed. Also, in the future please consider using the add suggestion to batch feature to co-author commits.

@kmilo9999
Copy link
Collaborator Author

I did use the suggestion to batch feature in the last 2 review following your advice

@kmilo9999
Copy link
Collaborator Author

@hollandjg everything good on your side?

Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🚀

@kmilo9999 kmilo9999 merged commit f0b3e2e into main Mar 13, 2024
2 checks passed
@kmilo9999 kmilo9999 deleted the clean-files-modules branch March 13, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants