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

Add/replace methods to be more Rust idiomatic #155

Closed
cjordan opened this issue Oct 13, 2022 · 5 comments · Fixed by #157
Closed

Add/replace methods to be more Rust idiomatic #155

cjordan opened this issue Oct 13, 2022 · 5 comments · Fixed by #157

Comments

@cjordan
Copy link
Contributor

cjordan commented Oct 13, 2022

Hi,

You asked for some suggestions on how to improve hifitime; I've found a couple!

The Rust API Guidelines provide some "rules" on how to name methods, whether into_, to_ or as_. Just now as I was poking some code, I realised that hifitime::Duration doesn't really follow this, and I think it would make the API better or at least more consistent with existing Rust APIs (e.g. Duration::in_seconds should be Duration::to_seconds). Another bugbear of mine is Duration::from_f64; this method could remain, but I think it would be nicer to use Duration::from_secs, Duration::from_millis etc., which is very similar to the standard library functions.

hifitime::Epoch technically is correct with its many as_ methods, although these methods borrow self when Epoch is already Copy (i.e. these methods don't need to borrow, and given the guidelines these methods should instead be named to_).

The #[deprecated = reason] annotation can be used to maintain semver compatibility while introducing any new methods.

What do you think? It would be a decent amount of work to vet all existing methods, but I think it would be worth it. There is no rush on this work as it's mostly a matter of taste, and I would be happy to pitch in but I'm likely too busy until early next year.

Once again, thanks for your help with this great library.

@ChristopherRabotin
Copy link
Member

Thanks for the feedback! You're entirely correct and I've thought of doing that. It's a bunch of repetitive work, but I might as well do it now so it doesn't have to be done at a later date. Version 3.5.0 will add the Python package, so I might as well make sure that folks using this moving forward don't get used to using the bad functions. Also, given all of the work I've done from 3.4.0 to this upcoming version, I hope I can take a short break from this and focus on ANISE and Nyx for a few months.

I'll get to it just after #154 .

@ChristopherRabotin
Copy link
Member

@cjordan on another note, for the UT1 time system, how do you currently use that? I suspect that you grab the EOP data from NASA, plug it into some software, and apply the TAI-UT1 offset to whatever your TAI truth is. Is that right?

Although I prefer hifitime to not require any input files, ANISE is definitely input-files based and the EOP parsing is expected to be there eventually for reference frame transformation.

@cjordan
Copy link
Contributor Author

cjordan commented Oct 13, 2022

Thanks for tackling this!

For UT1, yes, I'm (awkwardly) adding DUT1 (i.e. UT1-UTC) to our various Epoch structs. The values are actually generated by astropy but I assume it's very similar/the same as the source you quote. I appreciate that you don't want to require input files, and I think that's perfectly fine. In terms of adding/supporting UT1, I don't think it's a big deal for hifitime, and I'm not sure how to improve the situation.

@cjordan
Copy link
Contributor Author

cjordan commented Oct 15, 2022

This is great, thanks very much!

@ChristopherRabotin
Copy link
Member

ChristopherRabotin commented Oct 15, 2022 via email

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 a pull request may close this issue.

2 participants