RuedigerForster / appac

Atmospheric Pressure Peak Area Correction
3 stars 2 forks source link

Apply coding conventions [WIP] #30

Closed se-wo closed 1 month ago

se-wo commented 1 month ago

I started applying coding conventions to the code to improve readability and maintainability. I used Tidyverse style as the basis. I added a basic configuration for Lintr and configured a corresponding Github action. In addition I applied Styler to fix all automatically fixable issues.

Nevertheless there are still more than enough style related issues. Code style is to some extend art, so it´s difficult to argue whether or not tidyverse style is appealing. Style policy modifications can be made in .lintr. E.g.

linters: linters_with_defaults(
    line_length_linter(120)
    )
encoding: "UTF-8"

Current linter findings (Details)

Linter # Occurrences Comment
commented_code_linter 72 :x: CRAN package should not contain commented out code
cyclocomp_linter 1 :warning: No showstopper but could improve maintainability
line_length_linter 100 :warning: Maybe extend to 120 characters and fix rest?
object_name_linter 259 :information_source: Would be quite a lot of work
object_usage_linter 20 :x: Unused variables should be cleaned up
seq_linter 18 :warning: cause of errors if nrow/ncol is 0
T_and_F_symbol_linter 33 :information_source: But easy to fix
vector_logic_linter 2 :warning: Is using a single \| really intentional?
Total 505
se-wo commented 1 month ago

Open for discussion on how configure the linter rules

RuedigerForster commented 1 month ago

@se-wo: Your proposal is fine. Line length should not exceed 100 characters; I wlll try and keep it around 80. I will also fix commented_code and line_length, when the code is close to final. You don't need to do this. |: (simple or) was intentional: 2 logical expressions should be evaluated at development time. Can be replaced by || in final version. Can we have a short discussion about object_usage and object_name? My current scheme is: Classes and objects: UpperCamelCase methods: lowerCamelCase internal functions and their arguments: lower_snake_case exported functions and their arguments: lower.dot.case

RuedigerForster commented 1 month ago

@se-wo: Your proposal is fine. Line length should not exceed 100 characters; I wlll try and keep it around 80. I will also fix commented_code and line_length, when the code is close to final. You don't need to do this. |: (simple or) was intentional: 2 logical expressions should be evaluated at development time. Can be replaced by || in final version. Can we have a short discussion about object_usage and object_name? My current scheme is: Classes and objects: UpperCamelCase methods: lowerCamelCase internal functions and their arguments: lower_snake_case exported functions and their arguments: lower.dot.case

lintr is quite dumb. I did the best I could do. Left the dot.case for items in the S4 classes. Other names changed according to: https://cran.r-project.org/web/packages/easystats/vignettes/conventions.html

lintr does not recognize .not_exported_function, and variable names which are hidden in lists or function calls.

Commented code still there to some extent. Will delete later.

RuedigerForster commented 1 month ago

Added lintr workflow to source.