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

[doc] improve examples #692

Merged
merged 3 commits into from
Aug 28, 2020
Merged

[doc] improve examples #692

merged 3 commits into from
Aug 28, 2020

Conversation

pbougue
Copy link
Contributor

@pbougue pbougue commented Aug 27, 2020

fix #329

Also some clippy-related work.

@datanel datanel merged commit a64609c into hove-io:master Aug 28, 2020
Copy link
Contributor

@antoine-de antoine-de left a comment

Choose a reason for hiding this comment

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

nice!

But don't you think the link is a bit easy to miss and that some inline code in the doc would have more impact (with doc-comment, skeptic or whatewhere is the trend now 😜 )

);

// lines passing by stop
let lines = get(idx, &transit_objects.lines, &transit_objects);
Copy link
Contributor

Choose a reason for hiding this comment

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

completly minor, but don't you think that for the purpose of an example it would be more straightforward to directly use get_corresponding? (at least for the first ones)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True (especially if it goes in main readme), I tried to mix existing and suggestions 👯

@datanel
Copy link
Member

datanel commented Aug 28, 2020

But don't you think the link is a bit easy to miss and that some inline code in the doc would have more impact (with doc-comment, skeptic or whatewhere is the trend now )

@antoine-de get_corresponding is already well documented in https://github.com/CanalTP/relational_types/blob/master/src/lib.rs

Copy link
Contributor Author

@pbougue pbougue left a comment

Choose a reason for hiding this comment

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

@antoine-de I'm OK with your suggestion.
That said:

  • I don't think that lib examples should be too prominent compared to bin (maybe move examples below if they are in main readme)
  • I don't plan on spending much more time on this major doc refactor topic in the medium term
  • I'm OK to merge something that goes that direction (You know, open-source and all 😜 )

);

// lines passing by stop
let lines = get(idx, &transit_objects.lines, &transit_objects);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

True (especially if it goes in main readme), I tried to mix existing and suggestions 👯

@antoine-de
Copy link
Contributor

But don't you think the link is a bit easy to miss and that some inline code in the doc would have more impact (with doc-comment, skeptic or whatewhere is the trend now )

@antoine-de get_corresponding is already well documented in https://github.com/CanalTP/relational_types/blob/master/src/lib.rs

sure, but it's more difficult to discover as is.

@antoine-de I'm OK with your suggestion.
That said:

* I don't think that lib examples should be too prominent compared to bin (maybe move examples below if they are in main readme)

* I don't plan on spend much more time on this major doc refactor topic in the medium term

* I'm OK to merge something that goes that direction (You know, open-source and all 😜 )

honestly I don't plan on working on transit_model in the mid term, you can do whatever you want with my suggestion 😜

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.

Update Readme
3 participants