Skip to content

Conversation

@markrtucker
Copy link
Contributor

@markrtucker markrtucker commented Dec 16, 2025

The main change here is to add a manifest file for easy publishing to Connect.


This pull request makes minor improvements to the application configuration and code readability. The most notable change is the introduction of a new APP_CONFIG list for centralized application configuration, replacing the previous method of sourcing constants. Additionally, several code formatting adjustments improve readability, and the maximum allowed file size for pre-commit checks has been increased.

Configuration improvements:

  • Introduced a new APP_CONFIG list in app.R to centralize application configuration, such as the default data path, and removed the sourcing of constants.R.

Pre-commit configuration:

  • Increased the maximum allowed file size for the check-added-large-files pre-commit hook from 200KB to 500KB in .pre-commit-config.yaml.

Code readability and formatting:

  • Reformatted logical expressions and function arguments for better readability in several places within app.R, including checks for missing type values and calls to dplyr::mutate and ggplot2::scale_color_manual. [1] [2] [3]

@markrtucker markrtucker changed the base branch from main to dev December 16, 2025 12:19
hooks:
- id: check-added-large-files
args: ['--maxkb=200']
args: ['--maxkb=500']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The manifest file was over 200 KB. I'm not sure if this check is really necessary, but it came as part of the standard R checks.

)

# Load app constants
source("../constants.R")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When publishing to Connect from github, all files must be in the same parent directory, so we can't reference files at ... The constants.R file is not providing a ton of value, so I just moved the contents here.

has_type_na <- any(is.na(type_values) | type_values == "" | type_values == " ")
has_type_na <- any(
is.na(type_values) | type_values == "" | type_values == " "
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VSCode/Air did some reformatting...

@markrtucker markrtucker requested a review from Copilot December 16, 2025 12:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds a manifest file to support publishing to Connect and introduces centralized application configuration. The main change replaces the sourcing of a constants file with an APP_CONFIG list that defines the default data path. Additionally, the pre-commit hook maximum file size limit has been increased, and several code formatting improvements enhance readability.

  • Replaced source("../constants.R") with a new APP_CONFIG list for centralized configuration
  • Increased pre-commit file size limit from 200KB to 500KB
  • Improved code formatting by splitting long lines across multiple lines

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
inst/apps/connect/app.R Introduced APP_CONFIG list to replace constants sourcing and reformatted several function calls for better readability
inst/WORDLIST Added extensive list of valid terms for spell checking
.pre-commit-config.yaml Increased maximum allowed file size for large file checks from 200KB to 500KB

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@markrtucker
Copy link
Contributor Author

@matt-urbina - FYI only. I'm going to leave this as draft for now. My intention was to make it easy to deploy the shiny apps to connect.posit.it, but since we don't have curated data there, this doesn't help much.

I think we probably do want this long-term, but it's certainly not urgent to get this in now.

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.

2 participants