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

Better documentation for kappa-library #654

Open
jonathan-laurent opened this issue Mar 29, 2023 · 1 comment
Open

Better documentation for kappa-library #654

jonathan-laurent opened this issue Mar 29, 2023 · 1 comment

Comments

@jonathan-laurent
Copy link
Contributor

The documentation for kappa-library is lacking in many places and it would be great if we can launch a collective, incremental effort to improve it. I would be happy to submit PRs myself every time I get explained something or end up figuring it out by myself.

I am not talking about doing anything fancy here but simply making sure that all public-facing API functions are properly explained: what are the arguments (when nonobvious from the type), what is the returned value, what exceptions could be thrown, what are other related functions to be aware of...

For example, I am struggling right now with the absence of documentation for the following function in Edges.mli:

val get_connected_component : int -> t -> int option

Why does this return an option? When is this function expected to return None? Can it also raise exceptions (i.e. if connected components are not supported or the agent ID is invalid)?

@antoinepouille
Copy link
Contributor

Old issue, so not sure if you are still interested in this @jonathan-laurent

There's a lot of missing doc, I'm adding some incrementally on what I find the less clear. Still at the moment, you kinda have to dig in the code to understand how it goes

let get_connected_component ag graph =
  match graph.tables with
  | None -> assert false
  | Some tables ->
    (match tables.connected_component with
    | None ->
      raise
        (ExceptionDefn.Internal_Error
           (Loc.annot_with_dummy
              "get_connected_component while not tracking ccs"))
    | Some ccs -> Mods.DynArray.get ccs ag)

Here, there is indeed both an exception mechanism, an assert and an option (which comes if the agent isn't present in the css DynArray). It might make more sense to unify all these to a exception or an option, but I would need to understand why these choices were made, and how it was done across the surrounding code.

But then, I'm not fond of taking time to document this if that will change in a future revamp/cleanup.

However this poses the question of what is public-facing in the library here. To me it's not clear what can be safely changed, as it might break some code like the one you were writing…

What do you consider public-facing here? All mlis in core? You might have a clearer view than me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants