gbouras13 / dnaapler

Reorients assembled microbial sequences
MIT License
102 stars 3 forks source link

[JOSS Review] - Documentation #48

Closed vinisalazar closed 1 year ago

vinisalazar commented 1 year ago

Hi,

attached are some suggestions for the documentation based on my review:

https://github.com/gbouras13/dnaapler/blob/7c67b3beb930c9e2a1e031612841bc7541aa6915/README.md?plain=1#L20

https://github.com/gbouras13/dnaapler/blob/7c67b3beb930c9e2a1e031612841bc7541aa6915/README.md?plain=1#L106

https://github.com/gbouras13/dnaapler/blob/7c67b3beb930c9e2a1e031612841bc7541aa6915/docs/install.md?plain=1#L46


I will update this issue if I find anything else.

cc openjournals/joss-reviews#5968

gbouras13 commented 1 year ago

re https://github.com/openjournals/joss-reviews/issues/5968

Hi @vinisalazar ,

Thanks for these. Also linking to #51 regarding your third point as they will be dealt with together.

Your first point has been fixed in the README.md to make it easier for beginners to follow - namely, by making an empty conda env and then installing dnaapler (I did not get good results trying to do it all in one line, therefore a beginner will have a harder time!).

The second point also has been fixed.

To clarify, prodigal is not required in dnaapler at all. This is because pyrodigal is a self contained Python package that reimplements and provides python bindings to Prodigal and therefore does not require it as a dependency. pyrodigal will always be installed with a pip install of dnaapler.

Regarding your third point, install.md is overhauled after #51 - the correct numbering will be used.

George

mkerin commented 1 year ago

This is a quick comment to say that I think there is an unfinished sentence on line 54 of docs/example.md here https://github.com/gbouras13/dnaapler/blob/bf86601136951079218f258452f5ff40733d50f9/docs/example.md?plain=1#L54

I also think that, other than this typo, the new docs/example.md section is excellent.

gbouras13 commented 1 year ago

Hi @mkerin,

Thanks for picking this up - I've made a fix now in the commit references above.

George

vinisalazar commented 1 year ago

LGTM @gbouras13.

I will close this issue for now but @mkerin or @gbouras13 please reopen it if you deem it necessary.