Conversation
There was a problem hiding this comment.
Please add: #![deny(rustdoc::broken_intra_doc_links)] to the top of our lib.rs file.
Also add this to our ci:
doc-check:
name: Doc Check
runs-on: ubuntu-latest
timeout-minutes: 15
steps:
- uses: actions/checkout@v4
- uses: actions-rust-lang/setup-rust-toolchain@v1.13.0
with:
toolchain: stable
- uses: Swatinem/rust-cache@v2.8.0
- name: Check documentation
env:
RUSTDOCFLAGS: "-D warnings -D rustdoc::broken_intra_doc_links"
run: cargo doc --no-deps --all-features
| /// # Example | ||
| /// | ||
| /// The table | ||
| /// ```text |
There was a problem hiding this comment.
I think, this is:
| /// ```text | |
| /// ```csv |
| use std::collections::{HashMap, HashSet}; | ||
|
|
||
| #[derive(Debug)] | ||
| /// # Description |
There was a problem hiding this comment.
The first line of a doc string will be used in different places as a headline for this class on the doc website of crates.io.
For example:
https://docs.rs/securiety/0.2.8/securiety/curie/index.html
https://github.com/SmartMonkey-git/securiety/blob/c6eeb0b7aac212d331b0876dc6e284500ad1c2ad/src/curie.rs#L3
Can use see how the first line is used?
So, if we keep this we will see something like:
AgeToIso8601Strategy Description
| /// Given a column whose cells contains ages (e.g. subject age, age of death, age of onset) | ||
| /// this strategy converts integer entries to ISO8601 durations: 47 -> P47Y | ||
| /// | ||
| /// NOTE: the integers must be between 0 and 150. |
There was a problem hiding this comment.
I would:
| /// NOTE: the integers must be between 0 and 150. | |
| /// ## Note | |
| Integers must be between 0 and 150. |
or maybe leave it out here, since its already explained in the error section.
| #[derive(Debug)] | ||
| /// # Description | ||
| /// | ||
| /// Given a column whose cells contains ages (e.g. subject age, age of death, age of onset) |
There was a problem hiding this comment.
Please, refer to our types everywhere.
| /// Given a column whose cells contains ages (e.g. subject age, age of death, age of onset) | |
| /// Given a column whose cells contains ages (e.g. [`Context:SubjectAge`], [`Context:AgeOfDeath`], [`Context:AgeOfOnset`]) |
| /// and a ToString AliasMap which converts "M" to "Male" and "F" to "Female" | ||
| /// # Description | ||
| /// | ||
| /// Given a collection of `ContextualiseDataframes`, this strategy will apply all the aliases |
There was a problem hiding this comment.
Also here:
| /// Given a collection of `ContextualiseDataframes`, this strategy will apply all the aliases | |
| /// Given a collection of [`ContextualisedDataframe`], this strategy will apply all the aliases |
| /// P001, 4 | ||
| /// P002, 0 | ||
| /// ``` | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Errors will be thrown if: | ||
| /// - Any columns to be aliased cannot be cast to String datatype. |
There was a problem hiding this comment.
Also here: [String] or do you mean [Dtype::String]?
| /// # Errors | ||
| /// | ||
| /// An error will be thrown if | ||
| /// 1. A DOB is before to a date for a patient, leading to a negative age. |
There was a problem hiding this comment.
Here you use numbers on AliasMapStrategy you used dashes.
| /// 1. A DOB is before to a date for a patient, leading to a negative age. | |
| /// 1. A date of birth is before to a date for a patient, leading to a negative age. |
or might even link the context type.
| /// | ||
| /// An error will be thrown if | ||
| /// 1. A DOB is before to a date for a patient, leading to a negative age. | ||
| /// 2. There exists a date which cannot be converted to an age due to missing DOB data. |
There was a problem hiding this comment.
- There exists a date which cannot be converted to an age due to missing DOB data
I didn't know. This will be a major annoyance, as soon as we have large datasets.
There was a problem hiding this comment.
Yes. For those situations we have a few options:
- Just don't apply the strategy. I decided to be strict
- Add a bool to the strategy so it can be strict or otherwise
- Make the strategy just allow situations with missing data
Probably 2 is best. But I would point out that if you are creating a cohort of Phenopackets, you probably either want just ages in a field, or just dates. To have a mix is strange. So strictness did make sense. And DOB data probably is less likely to be missing than other data. Miraculously the strategy seemed to work with the i_data.
| /// # Fields | ||
| /// | ||
| /// * `hpo_bidict_lib` - This should contain BiDictLibrary for the version of HPO that you want to use. | ||
| /// * `disease_bidict_lib` - All non-HPO cells will be processed by this disease BiDictLibrary. |
There was a problem hiding this comment.
I don't think Fields is a common section. At least not to the standard.
Maybe we should drop it.
| /// ```ignore | ||
| /// let sex_mapping = MappingStrategy::default_sex_mapping_strategy(); | ||
| /// // Maps variations like "m", "male", "man" → "MALE" | ||
| /// // and "f", "female", "woman" → "FEMALE" | ||
| /// ``` |
There was a problem hiding this comment.
Example is not concise with the others
2 participants
No description provided.