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

Support for astronomical magnitudes #9

Merged
merged 13 commits into from
Apr 8, 2019
Merged

Support for astronomical magnitudes #9

merged 13 commits into from
Apr 8, 2019

Conversation

m-wells
Copy link
Contributor

@m-wells m-wells commented Mar 29, 2019

This closes issue #8. I've added unit tests, updated the docs to include usage examples, and updated the readme. I even handle color index.

@m-wells m-wells mentioned this pull request Mar 29, 2019
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

Wow, a first PR with full documentation and tests, what a dream!

src/UnitfulAstro.jl Show resolved Hide resolved
src/UnitfulAstro.jl Show resolved Hide resolved
src/UnitfulAstro.jl Outdated Show resolved Hide resolved
src/UnitfulAstro.jl Outdated Show resolved Hide resolved
@m-wells
Copy link
Contributor Author

m-wells commented Mar 29, 2019

Alright. I think I addressed all the concerns @giordano had. I removed two lines that were no longer needed. Unless there is any other concerns, I think this is ready 😃

@m-wells
Copy link
Contributor Author

m-wells commented Apr 5, 2019

Are we just waiting for @mweastwood to review?

@giordano
Copy link
Member

giordano commented Apr 5, 2019

Yep, I'd like him to have a look, since he's the author and maintainer so far of the package.

Copy link
Member

@mweastwood mweastwood left a comment

Choose a reason for hiding this comment

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

Apologies for the slow review, my new job doesn't let me spend as much time on Julia as before.

This looks great, thanks for tackling this! The documentation looks really good too.

@m-wells
Copy link
Contributor Author

m-wells commented Apr 8, 2019

Excellent! Can we merge now? 😃

@giordano giordano merged commit d84c518 into JuliaAstro:master Apr 8, 2019
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.

3 participants