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

load.project issues #76

Merged
merged 28 commits into from
Jun 3, 2014
Merged

load.project issues #76

merged 28 commits into from
Jun 3, 2014

Conversation

krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Apr 10, 2014

  • Use $ to access config options
  • Use $ to access my.project.info
  • Get default configuration as list
  • Redundant code for checking for missing entries in config: Extract to function
  • Options that allow on/off should be converted to logical
  • Use pattern parameter in dir()
  • Vectorize skipping of deprecated files
  • Make use of Boolean configuration options
  • Use require.package
  • Create necessary directories (cf. Optionally, create.project() should create dummy files in empty directories #35)
  • warn instead of stopping if configuration file is missing
  • Redundant code for loading from cache
  • Extract functions, one per step
  • allow overriding (some of the) configuration options by (a) new parameter(s)
  • warn on extra entries in the configuration
  • avoid duplication of global.dcf (Avoid duplication of global.dcf #79)

@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 10, 2014

Converted from issue using hub pull-request -i 76. Work in progress.

@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 17, 2014

Ready for review.

for (helper.script in dir('lib'))
helpers <- dir('lib', pattern = '[.][rR]$')
deprecated.files <- intersect(
helpers, c('boot.R', 'load_data.R', 'load_libraries.R',
Copy link
Owner

Choose a reason for hiding this comment

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

Can we define deprecated files in an external file. Then as files are deprecated in the future won't need to hunt for them in source code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you sure we need it? An external file adds an extra level of complexity, and it might be more difficult to find it. It's only a few files, after all, and the list is not likely to become much larger.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, it might be a bit of YAGNI, especially if the file list might not change. Perhaps pull it into a function? Can probably be deferred until we need to update the list.

@krlmlr krlmlr mentioned this pull request Apr 17, 2014
@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 17, 2014

Do you think we need 0570eb7 (ad-hoc change of configuration) if we can have multiple config files?

@KentonWhite
Copy link
Owner

I think it should be kept. One of the annoyances of rails is having to create a new configuration file just to launch with a simple change. I like that this commit can let me override config variables for a quick one off. Multiple config files are then useful for settings I use frequently.

@krlmlr
Copy link
Collaborator Author

krlmlr commented Jun 3, 2014

Addressed all issues, will merge later today. Thanks for the review.

krlmlr added a commit that referenced this pull request Jun 3, 2014
@krlmlr krlmlr merged commit 0c3438f into master Jun 3, 2014
@krlmlr krlmlr deleted the 76-load-project branch June 3, 2014 19:15
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.

None yet

2 participants