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

Fix incomplete links in documentation #217

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zimeg
Copy link
Contributor

@zimeg zimeg commented May 5, 2023

Summary

👋 Hello! This PR updates incomplete links in the documentation to point to the expected page.

Updated links

Here are the anchors to where these updates begin for each page:

Notes and questions

I'm slightly unsure of where the links in this section should lead, but am willing to make any suggested updates!

Formatting of the ocl::core::Status link is also troublesome and doesn't seem to want to appear inline... Would it make sense to add another line to reference these docs?

/// Returns an `Err(ocl::core::Error::Status {...})` enum variant upon any
/// OpenCL error. Calling [`.status()`] on the returned error will return
/// an `Option(``[ocl::core::Status]``)` which can be unwrapped then
/// matched to determine the precise reason for failure.
///
/// [`.status()`]: enum.Error.html#method.status
/// [`ocl::core::Status`]: enum.Status.html

@c0gent
Copy link
Member

c0gent commented May 6, 2023

Looks great.

Formatting of the ocl::core::Status link is also troublesome and doesn't seem to want to appear inline... Would it make sense to add another line to reference these docs?

Yes, that sounds good or whatever you think would be clearest.

I'm slightly unsure of where the links in this section should lead, but am willing to make any suggested updates!

Sorry which links?

@zimeg
Copy link
Contributor Author

zimeg commented May 8, 2023

Thanks! Before making the inline link change, I wanted to share an answer and some findings for the following question:

Sorry which links?

For the ::list method, the links to .status() and ocl::core::Status are both pointing to missing links. I believe these resources might have moved in a refactoring of ocl_core.

Digging into the code, I think possible errors for ::list are of type ocl_core::functions::ApiError which implements .status()1, but this method might not have link-able documentation. If this seems correct, I can add and link to this documentation?

And at a glance, it seems that the ocl::core::Status is now represented as ocl_core::Status. Replacing this link and related text (ocl::core::Error::Status) should be good, but I'm not fully sure...

Based on the implementation of .status(), I'm also wondering if the following change should be made, removing the Option around Status2:

  Calling `.status()` on the returned error will return an 
- `Option(ocl::core::Status)` which can be unwrapped then
+ `ocl_core::Status` which can be
  matched to determine the precise reason for failure.

Not meaning to turn these link updates into a code exploration, I'm just somewhat unfamiliar with the multi-crate structure 😅

Footnotes

  1. ::list calls core::get_device_ids which sometimes returns an error from eval_errcode, which can be an ocl_core::functions::ApiError.

  2. I believe the Option<Status> is instead returned by .api_status() of ocl_core::error::Error

@c0gent
Copy link
Member

c0gent commented May 8, 2023

Ok great. It's been a while, I'll have to refamiliarize myself when I have time. I'll get back to you as soon as I can.

Appreciate your help!

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