JuliaText / TextAnalysis.jl

Julia package for text analysis
Other
373 stars 95 forks source link

Add offline Documentation (Docstrings) to the codebase #150

Closed Ayushk4 closed 5 years ago

Ayushk4 commented 5 years ago

I am adding docstrings. I will work my way starting with the lexicographically largest down to smallest based on file names, (i.e. in the following order), so that @aquatiko can work in lexigraphical order in #147 , preventing overlaps.

Edit: closes #146

aquatiko commented 5 years ago

sa.jl, lda.jl, hash.jl, dtm.jl is done

Ayushk4 commented 5 years ago

Apart from adding the docstrings (offline docs), I have also added the documentation for the missing functions for tf.

Ayushk4 commented 5 years ago

I am done adding DocStrings. This PR is ready to be reviewed.

laschuet commented 5 years ago

Thank you very much for all the work you both put into this PR @Ayushk4 and @aquatiko!

It may be petty, but I would follow the recommendations in the official Julia documentation and prefer the imperative form instead of the third person (e.g. "compute" vs. "computes") for the one-line sentences. This would also be more in line with the official Julia code documentation (e.g. base, stdlib).

Ayushk4 commented 5 years ago

Sure, @laschuet . I will make the changes tomorrow.

Ayushk4 commented 5 years ago

Changes done! @laschuet

laschuet commented 5 years ago

Thank you very much!

If I may be petty again:

  1. Some one-line sentences end with a period (as recommended by the official Julia documentation) and some do not. Overall consistency would be great.
  2. Based on the official Julia documentation (again :smiley:)), more details should follow in a second paragraph after the one-line sentences (after a blank line). Currently, this is not always the case.

Besides that, do we also want to integrate doctests? I don't know how this works in detail, but the doctests should be run by default (based on our current documentation configuration) if there are any. This is about jldoctest blocks versus julia-repl blocks.

Ayushk4 commented 5 years ago
  1. Thanks for pointing out @laschuet . I missed out period in metadata.jl. I will update it soon. :smile:

  2. I will take it into consideration and make the required changes.

  3. I went for julia-repl instead jldoctests, to ensure consistency with #147, that uses the former. If we are going with the latter, then I will send a PR to the branch in #147 (aquatiko::docAPI) as well, alongside updating this. So, should we go for jldoctests for the entire codebase then?

Also, thank you so much for taking out time to give suggestions and reviewing this. :+1:

laschuet commented 5 years ago

In the longer term, I would prefer doctests. This could also be integrated later (new PR) as soon as the current documentation changes are merged. But I don't think I'm in a position to decide that.

Ayushk4 commented 5 years ago

@laschuet Changes have been made.

laschuet commented 5 years ago

Thank you very much once again, @Ayushk4! I like that this PR is improving with every iteration :+1:.

Another "issue" I spotted is the following inconcistency regarding the function signatures: Sometimes you provide the full function signature including the types for every argument, and sometimes you leave the type information out and "only" provide the argument names.

Ayushk4 commented 5 years ago

I am not mentioning types, unless the variable names aren't conveying the message like following cases - crps from Corpus, str for String, etc.

laschuet commented 5 years ago

Thank you for the clarification. That is one way to do it. What about titles!(crps, str::String) and title!(doc, str)?

Ayushk4 commented 5 years ago

I will change that. Thanks for pointing it out.

aquatiko commented 5 years ago

I got little left behind on this, will be improving #147 very soon. Actually, I was waiting for some suggestions from people out there and didn't knew about the official Julia Documentation. Thnx @laschuet for pointing it!

aviks commented 5 years ago

@Ayushk4 Is this ready to merge? Looks good from my perspective.

Ayushk4 commented 5 years ago

With #155 merged, I need to go through the all the # Examples sections in docstrings and update it. I will make the changes by today itself and ~inform~ notify when it's done.

Ayushk4 commented 5 years ago

@aviks I am done making the changes. This should be ready to merge. Please review / suggest changes, if any.