Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint etl subdirectories #677

Merged
merged 15 commits into from
Dec 11, 2024
Merged

Lint etl subdirectories #677

merged 15 commits into from
Dec 11, 2024

Conversation

wrridgeway
Copy link
Member

@wrridgeway wrridgeway commented Dec 10, 2024

This PR applies styler::style_dir() to scripts-ccao-data-raw-us-east-1 and scripts-ccao-data-warehouse-us-east-1.

@wrridgeway wrridgeway marked this pull request as ready for review December 10, 2024 20:33
@wrridgeway wrridgeway requested a review from a team as a code owner December 10, 2024 20:33
@wrridgeway wrridgeway changed the title Linting Lint etl subdirectories Dec 10, 2024
Copy link
Member

@dfsnow dfsnow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with making this kind of massive PR if it's just linting. However, let's check that the line endings of all these files are the same (unix line endings).

@wrridgeway
Copy link
Member Author

Yeah, everything seems to LF now.

@wrridgeway wrridgeway requested a review from dfsnow December 10, 2024 22:25
Copy link
Contributor

@jeancochrane jeancochrane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An annoying change, but a super helpful one nonetheless. Thanks for picking up this chore!

Comment on lines +636 to +642
# nolint start
# pdf(str_c("output/krig_demo/", year_num, ".pdf"), width = 11, height = 8.5)

#plot_surface(k)

#dev.off()
# plot_surface(k)

# dev.off()
# # nolint end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nitpick, non-blocking] Rather than turn off linting for this block, maybe we just delete the unused code that's commented out?

Comment on lines +749 to +750
# raster_location <- str_c("output/kriging_surfaces/rasters/", year, ".tif") # nolint
# raster_file <- read_stars(raster_location) # nolint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nitpick, non-blocking]

Suggested change
# raster_location <- str_c("output/kriging_surfaces/rasters/", year, ".tif") # nolint
# raster_file <- read_stars(raster_location) # nolint
# raster_location <- str_c("output/kriging_surfaces/rasters/", year, ".tif") # nolint
# raster_file <- read_stars(raster_location) # nolint

Comment on lines +694 to +695
# Demo from 2010:
# plot_surface(out[[1]]) # nolint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nitpick, non-blocking] Same here:

Suggested change
# Demo from 2010:
# plot_surface(out[[1]]) # nolint

Copy link
Member Author

@wrridgeway wrridgeway Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right that this should get removed, but this entire file needs to be cleaned up for extraneous code. I only wanted to address formatting in this PR so that there wouldn't be any uncertainty as to whether the insane number of changes here might lead to any changes in outputs or how code is understood.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wrridgeway wrridgeway merged commit dc41cf4 into master Dec 11, 2024
6 checks passed
@wrridgeway wrridgeway deleted the code-cleaning branch December 11, 2024 17:20
@jeancochrane jeancochrane mentioned this pull request Dec 20, 2024
82 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants