search

pfmc-assessments / PacFIN.Utilities

R code to manipulate data from the PacFIN database for assessments
http://pfmc-assessments.github.io/PacFIN.Utilities
Other
7 stars 1 forks source link

Use {styler} to style all code in {PacFIN.Utilities}? #94

Closed iantaylor-NOAA closed 1 year ago

iantaylor-NOAA commented 1 year ago

Is your feature request related to a problem? Please describe. Some of the code is harder to read and understand because of all the normal things that occur when code evolves over a long period of time: inconsistent spacing and linebreaks, use of single vs double quotes, use of = vs <-, etc.

While trying to debug a recent error, I searched for all instances of "sexed" which occurred in the error, but the issue was actually with a line that included 'sexed' with single quotes.

Describe the solution you'd like Running styler::style_pkg() cleans all this up. I looked through the changes to two functions and they all seemed like improvements to me (but I'm used to looking at code that {styler} has modified).

This could either be done once or repeatedly via github action that is easy to set up by running either ghactions4r::use_style_r_code() or ghactions4r::use_doc_and_style_r(use_rm_dollar_sign = FALSE)

Describe alternatives you've considered Stick with the status-quo code.

Additional context We could have a larger conversation about the use of {styler} within all pfmc-assessments packages, I'm just focused on this one right now.

andi-stephens-NOAA commented 1 year ago

I didn't know about styler. Sounds like a good idea!

In the olden days, we had style guides, but no way to enforce consistency. Just let me tell you how I feel about tabs vs spaces....

On Thu, Feb 16, 2023 at 3:27 PM Ian Taylor @.***> wrote:

Is your feature request related to a problem? Please describe. Some of the code is harder to read and understand because of all the normal things that occur when code evolves over a long period of time: inconsistent spacing and linebreaks, use of single vs double quotes, use of = vs <-, etc.

While trying to debug a recent error, I searched for all instances of "sexed" which occurred in the error, but the issue was actually with a line that included 'sexed' with single quotes.

Describe the solution you'd like Running styler::style_pkg() cleans all this up. I looked through the changes to two functions and they all seemed like improvements to me (but I'm used to looking at code that {styler} has modified).

This could either be done once or repeatedly via github action that is easy to set up by running either ghactions4r::use_style_r_code() or ghactions4r::use_doc_and_style_r(use_rm_dollar_sign = FALSE)

Describe alternatives you've considered Stick with the status-quo code.

Additional context We could have a larger conversation about the use of {styler} within all pfmc-assessments packages, I'm just focused on this one right now.

— Reply to this email directly, view it on GitHub https://github.com/pfmc-assessments/PacFIN.Utilities/issues/94, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACTUYBV75OBYHXAPTORAUYTWX2ZVHANCNFSM6AAAAAAU6Y6UWI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

iantaylor-NOAA commented 1 year ago

In case it's helpful, here's an example of the initial impact of running {styler} on a package with messy code: https://github.com/r4ss/r4ss/pull/447/files#diff-ce5fa22fa06ed2420b4443f07d1b94b8175bd09ae259b487ac4f800ae41cd93b

kellijohnson-NOAA commented 1 year ago

@iantaylor-NOAA I really appreciate you being willing to debug PacFin.Utilities right now while I am tied up with Pacific Hake and any time really 🤣 . But, I do not want to run a styler on PacFIN because I believe 50% of the code will be deprecated and is not worth putting time into cleaning up. So, as painful as it is, just fix things that bug you as you work on sections. Sorry to have that attitude but I do not want to spend time ensuring that everything works after running a styler on it and I do not have automated testing in place that I feel confident in to ensure that everything will work afterwards.

I want to emphasize here that I agree stylers are the way to go and I would like to also get there for every package in this organization but right now is not the time for this package.

iantaylor-NOAA commented 1 year ago

That makes sense @kellijohnson-NOAA. I think that I'll be done thinking about PacFIN.Utilities for a while anyway now that it is working fine for petrale.