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

Document #293 in local-offset feature description #297

Closed
wants to merge 1 commit into from

Conversation

jcdyer
Copy link
Contributor

@jcdyer jcdyer commented Dec 18, 2020

This PR adds documentation to the feature flag for local-offset indicating that the feature is useless on unix platforms, with a reference to #293.

I just spent longer than I wanted trying to figure out why OffsetDateTime::try_local_now was returning IndeterminateDateTime. The path I had to take to figure it out:

  1. Go to the documentation for OffsetDateTime::try_now_local. No mention of the issue.
  2. Check the source for that method. No mention of the issue.
  3. Go to the documentation for UtcOffset::local_offset_at. No mention.
  4. Find the undocumented private function try_local_offset_at. Reference to The call to localtime_r may be unsound #293.
  5. Check github for The call to localtime_r may be unsound #293. Issue is marked resolved.
  6. Switch dependency to main. Discover feature flag and new method names. Error persists.
  7. Repeat 1-4 with new nomenclature. Same reference.

Hopefully a mention on the documentation for the feature flag will prevent others from having to follow that same path.

@jcdyer
Copy link
Contributor Author

jcdyer commented Dec 18, 2020

I'd be happy to also add documentation to the same effect directly on all the #[cfg(feature = "local-offset")] methods.

@dekellum
Copy link

See also #296. IMO, thats the one that shouldn't have been closed.

@jhpratt
Copy link
Member

jhpratt commented Dec 18, 2020

To be clear: #293 is technically resolved. The call is no longer unsound, and will no longer randomly crash your program. #296 was more of a question, and was closed accordingly.

In will look at this PR later today. Adding a note in documentation has been planned; I've just been preoccupied.

@jcdyer
Copy link
Contributor Author

jcdyer commented Dec 18, 2020

I can update this to point to 296. The comment in the code mentions 293, so I followed suit.

@jhpratt
Copy link
Member

jhpratt commented Dec 18, 2020

Looks like I already added the note to documentation in 288c942. Unless there's something I missed, I believe this should be closed. The feature flag itself shouldn't need to be documented as such, as it's the individual methods that people will be looking at primarily.

@jhpratt jhpratt added the A-docs Area: documentation label Dec 18, 2020
@jcdyer jcdyer closed this Dec 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants