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/add doc string #282

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Fix/add doc string #282

merged 2 commits into from
Oct 23, 2023

Conversation

niyarin
Copy link
Contributor

@niyarin niyarin commented Jul 12, 2023

No description provided.

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #282 (b0c47bc) into master (3966774) will decrease coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 93.24%.

@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
- Coverage   88.86%   88.84%   -0.02%     
==========================================
  Files          79       79              
  Lines        6772     6778       +6     
  Branches      475      475              
==========================================
+ Hits         6018     6022       +4     
- Misses        279      281       +2     
  Partials      475      475              
Files Coverage Δ
src/cljam/algo/depth.clj 75.28% <100.00%> (ø)
src/cljam/algo/pileup.clj 96.63% <ø> (ø)
src/cljam/algo/sorter.clj 92.23% <100.00%> (+0.07%) ⬆️
src/cljam/io/bam/common.clj 100.00% <100.00%> (ø)
src/cljam/io/bam/core.clj 97.05% <ø> (ø)
src/cljam/io/bam/encoder.clj 70.70% <100.00%> (ø)
src/cljam/io/bam/writer.clj 92.77% <100.00%> (ø)
src/cljam/io/bam_index/common.clj 100.00% <100.00%> (ø)
src/cljam/io/bam_index/core.clj 82.05% <100.00%> (ø)
src/cljam/io/bam_index/reader.clj 97.70% <100.00%> (ø)
... and 34 more

@niyarin niyarin marked this pull request as ready for review October 4, 2023 07:31
@niyarin niyarin requested review from alumi and a team as code owners October 4, 2023 07:31
@niyarin niyarin requested review from r6eve and removed request for a team October 4, 2023 07:31
@alumi alumi assigned alumi and r6eve Oct 6, 2023
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thank you for adding the docstrings, @niyarin!
Adding documentation across the entire library must be a tough task. 🙏
Overall, I find it good, but I thought there's room for improvement in a few areas.
I believe it's a great opportunity to reduce the psychological barriers to using the library for new users 🉐

  • Enclose symbols like arguments in backticks ` `.
  • In cases like the following, provide more detailed information on return values:
    • When the implementation is not immediately clear.
    • When it's not intuitive from the namespace and function names.
    • When it might require referring to tests.
  • Address some grammatical issues, including
    • punctuation
    • capitalization
    • third-person singular '-s'
    • missing 'a' or 'the'

src/cljam/io/bam/common.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/common.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/core.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/decoder.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/encoder.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/header.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/header.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/option.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/option.clj Outdated Show resolved Hide resolved
src/cljam/io/sam/util/option.clj Outdated Show resolved Hide resolved
@niyarin niyarin force-pushed the fix/add-doc-string branch 3 times, most recently from db66605 to b762806 Compare October 16, 2023 22:22
@niyarin niyarin requested a review from alumi October 16, 2023 22:49
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks!

Copy link
Contributor

@r6eve r6eve left a comment

Choose a reason for hiding this comment

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

LGTM except the minor typos. After fixing that, could you squash the commits and rebase to master?

(def ^:const linear-index-shift 14)
(def ^:const linear-index-depth 5)
(def ^:const linear-index-shift
"Minimum shift size for searching binnning."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Minimum shift size for searching binnning."
"Minimum shift size for searching binning."

(defn writer [f {:keys [cols create-index?]
:or {cols 80 create-index? true}}]
(defn writer
"Returns an open `cljam.io.fasta.writer.FATAWriter` of `f`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Returns an open `cljam.io.fasta.writer.FATAWriter` of `f`.
"Returns an open `cljam.io.fasta.writer.FASTAWriter` of `f`.


(defn phred-bytes->fastq
"Convertsphred bytes to fastq quality string."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Convertsphred bytes to fastq quality string."
"Converts phred bytes to fastq quality string."

@@ -119,6 +120,7 @@
v)]))

(defn load-meta-info
"Reads from rdr and parases them as meta-info."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Reads from rdr and parases them as meta-info."
"Reads from `rdr` and parses them as meta-info."

[line]
(cstr/split (subs line 1) #"\t"))

(defn load-header
"Reads from `rdr` and parases them as header."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Reads from `rdr` and parases them as header."
"Reads from `rdr` and parses them as header."

@@ -4,6 +4,8 @@
[proton.core :as proton]))

(defn normalize-name
"Converts characters ',' or '.' in chromesome name to '_'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Converts characters ',' or '.' in chromesome name to '_'
"Converts characters ',' or '.' in chromosome name to '_'

@niyarin
Copy link
Contributor Author

niyarin commented Oct 23, 2023

Thank you for reviewing.
I rebased and fixed typo.

@r6eve r6eve merged commit b7f7fa1 into master Oct 23, 2023
17 checks passed
@r6eve r6eve deleted the fix/add-doc-string branch October 23, 2023 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants