dokato / todor

TODOr - RStudio add-in for finding TODO, FIXME, CHANGED etc. comments in your code.
Other
246 stars 12 forks source link

Enable todor() to produce nice markdown output #44

Closed mcguinlu closed 3 years ago

mcguinlu commented 3 years ago

(Partially) fixes #38.

Hi @dokato, I implemented a fix for this issue for my own use, so said I would open a PR in case it is helpful to anyone else.

In the new implementation, an additional argument (output) has been added to the todor() function which defines the format of the results. Using this approach, the inline expression r todor::todor(output = "text") in an RMarkdown file now produces the following output in the rendered document (when applied to the todor package):

Screenshot 2021-05-12 130052

ToDos are grouped by filename, which is bolded for ease of reference.

At present, the output argument only accepts "text" (which returns markdown text) as a value. A possible further iternation could be "dataframe", which would allow users to asign the results of todor to a dataframe with the columns "file", "type" and "message".

Very happy to add extra functionality/documention/tests as part of the PR process - just let me know!

dokato commented 3 years ago

Fantastic! Thanks a lot @mcguinlu 👍 I was just going to answer below this issue, that I might find time on that over the weekend ;) I had a first look, looks great. I'll test that later today, or tomorrow and come back with comments if I have any.

mcguinlu commented 3 years ago

Have left some comments on your requests - all very doable, and will have a look at implementing!

One final query is that when I run devtools::document() on the package, it creates .Rd files for exported function in man/ that are missing in your repo - is there a reason why you don't have a todor_file.Rd file in man/ for example?

dokato commented 3 years ago

One final query is that when I run devtools::document() on the package, it creates .Rd files for exported function in man/ that are missing in your repo - is there a reason why you don't have a todor_file.Rd file in man/ for example?

Great! I simply forgot to update Rd after major changes last year, I actually fixed that in #46 , so maybe to avoid merging conflicts you can skip it here. Once we merge in these two, I'll rebuild the docs separately.

mcguinlu commented 3 years ago

Hi @dokato,

All changes made now.

The only thing that might cause a flag for you is the use of return() when returning the list of markers. For some reason (you might know, but I don't) if I didn't use return() here, a NULL value was returned when using output = "list".

I haven't added any tests, as I wasn't sure if you wanted to do that centrally (similar to how you plan to rebuild the docs), but let me know if you need me to add some to ensure the new functionality is working correctly. Similarly, let me know if there are any further changes needed!

dokato commented 3 years ago

Fantastic, thanks @mcguinlu . I'll take care of the tests myself, though looks like a busy week ahead of me, but with some polishing touches over the weekend, I should bump up the version and update on CRAN soon.

The only thing that might cause a flag for you is the use of return() when returning the list of markers. For some reason (you might know, but I don't) if I didn't use return() here, a NULL value was returned when using output = "list".

Neglecting return only works on the variables that are at the very end of a function, so where you left it is as it should be.