search

thijsjanzen / nLTT

Repository for the R nLTT package
GNU General Public License v2.0
6 stars 4 forks source link

Fix lintr warnings #52

Closed richelbilderbeek closed 4 years ago

richelbilderbeek commented 4 years ago

Hi @thijsjanzen,

as can be seen in Pull Request #51, our friend @lintr-bot still has some suggestions to improve nLTT even more. Me and @Neves-P both try to get a clean lint, but we may not see everything anymore die to many suggestions in the ABC part of the code.

Would be great if you'd clean up your part of the code!

No rush, but it would make people and bots happy :+1:

Cheers, Richel

thijsjanzen commented 4 years ago

I have fixed most of @lintr-bot comments (see the short list of comments at https://github.com/thijsjanzen/nLTT/commit/e92e53a40f16c0cc4c3344820701e2a08a6da756), except some cyclomatic complexity issues that I do not have the time for, and that I also don't think are relevant right now - furthermore, the ABC and MCMC code exists mainly as a repository for the code used in the original nLTT paper. Fixing the cyclomatic complexity for these functions would force a re-write, and break continuity: the new less cyclomatic code would no longer resemble the code used for the paper.

Perhaps we can # nolint these functions?

richelbilderbeek commented 4 years ago

Well done!

I see your point about the ABC functions.

Using # nolint is a slippery slope, so if these functions are not part of the nLTT functionality, I would prefer to move them, for example either to scripts/paper.R or a repo like thijsjanzen/nltt_paper.

Well, in the end, you are the owner here, feel free to ignore my opinion. :+1:

thijsjanzen commented 4 years ago

Well done!

I see your point about the ABC functions.

Using # nolint is a slippery slope, so if these functions are not part of the nLTT functionality, I would prefer to move them, for example either to scripts/paper.R or a repo like thijsjanzen/nltt_paper.

Well, in the end, you are the owner here, feel free to ignore my opinion. 👍

I see your point, and I think nolinting the entire file is not really desirable.

On the other hand, moving it to scripts also seems undesirable, because this causes exclusion from the PDF manual, and breaks all tests of this code. Similarly, moving to another repo also makes it harder for users to find this code, and that should be avoided.

For now, it seems we'll have to live with these two mentions of @lintr-bot.

richelbilderbeek commented 4 years ago

Nah, I know a trick to exclude linting from files... just you wait for the magic to happen :rainbow:

richelbilderbeek commented 4 years ago

Oops, committed on master :pray:

richelbilderbeek commented 4 years ago

Works, only four messages left:

R/get_average_nltt_matrix.R:14:1: style: functions should have cyclomatic complexity of less than 15, this has 16.

get_average_nltt_matrix <- function(

^

R/nltt_diff_exact_extinct.R:47:1: style: functions should have cyclomatic complexity of less than 15, this has 23.

nltt_diff_exact_extinct <- function(

^

R/nltts_plot.R:19:1: style: functions should have cyclomatic complexity of less than 15, this has 17.

nltts_plot <- function(

^

R/stretch_nltt_matrix.R:22:1: style: functions should have cyclomatic complexity of less than 15, this has 16.

stretch_nltt_matrix <- function(

I suggest to let the author of each function simplify it. AFAICS, two are mine and two are of the DAISIE team.

After fixing this, @thijsjanzen, can we put an enforced lint on the package (as done, for example, here)?

thijsjanzen commented 4 years ago

I suggest to let the author of each function simplify it. AFAICS, two are mine and two are of the DAISIE team.

After fixing this, @thijsjanzen, can we put an enforced lint on the package (as done, for example, here)?

Agreed! Juicy!

richelbilderbeek commented 4 years ago

I will fix my functions this Saturday, after that I'll contact the other authors: no need to rush.

thijsjanzen commented 4 years ago

I will fix my functions this Saturday, after that I'll contact the other authors: no need to rush.

Then, why not fix it on Monday? It's weekend after all! No rush at all needed here, these issues are very, very minor.

Neves-P commented 4 years ago

Hi both! Sounds like a great plan, I'll work on it 👍

richelbilderbeek commented 4 years ago

OK, here is the distribution of labor:

richelbilderbeek commented 4 years ago

Done! Also a clean lint is enforced now :+1: