load project-specific configuration as well as user-specific #1469
Conversation
so that you can configure a plugin per-project
There was a problem hiding this comment.
Added a few specific comments that would bar this from being accepted. In general I'm noticing quite a few breaking changes which are a blocker for merging. There are also a few deprecations, many of which do not seem necessary. It seems to me like project based configuration could be implemented in a completely additive fashion rather than changing any existing behaviors.
Overall, however, I think a little bit more thought needs to be put into the semantics of this feature for it to be valuable to various users / use cases. As it stands, it seems as though "project directory" is loosely defined by this PR as the current working directory, which is not exactly the most reliable way to do this. I think at the very least, this would need to be documented, but more accurately, I don't think you can really define a "project directory" without the ability to customize said directory. That makes this PR a little bit harder to build, as it would (ideally) require CLI options for users to configure / customize where their "project" is. Also, as a small sidenote here, I notice you only support the config file and not the full directory, which I think some users may have an issue with. Supporting this approach would lead to a bigger maintenance burden than implementing it correctly the first time around.
Another more generalized approach to this problem would be to simply go the node_modules route and search for a .yard directory in each parent directory to the root, followed by a ~/.yard search. This would be compatible with eventually supporting #1230 as a final lookup location.
tl;dr simply implementing better lookup semantics for the .yard config directory would probably solve a lot of the needs for a project specific environment. Alternatively, simply having a CLI options that is also configurable via environment variable that points to said config dir / file / etc would probably solve 99% of the use cases here, even without directory lookup semantics.
| USER_CONFIG_DIR = File.expand_path('~/.yard') | ||
|
|
||
| # The main configuration YAML file. | ||
| CONFIG_FILE = File.join(CONFIG_DIR, 'config') | ||
| # @deprecated Use {USER_CONFIG_DIR} | ||
| CONFIG_DIR = USER_CONFIG_DIR |
There was a problem hiding this comment.
Not sure this is a necessary deprecation-- it seems arbitrary to name this USER_CONFIG_DIR when there does not seem to be a "project config directory" equivalent anyway.
|
|
||
| # File listing all ignored plugins | ||
| # @deprecated Set `ignored_plugins` in the {CONFIG_FILE} instead. | ||
| IGNORED_PLUGINS = File.join(CONFIG_DIR, 'ignored_plugins') | ||
| IGNORED_PLUGINS = File.join(USER_CONFIG_DIR, 'ignored_plugins') |
There was a problem hiding this comment.
It seems as though IGNORED_PLUGINS would not support project configuration? Seems like an omission in functionality.
| def self.read_config_file | ||
| if File.file?(CONFIG_FILE) | ||
| # @see config_file | ||
| def self.read_config_file(config_file) |
There was a problem hiding this comment.
Requiring a parameter would be a breaking change. Why not default to using CONFIG_FILE?
| def self.config_file(dir) | ||
| File.join(dir, 'config') | ||
| end | ||
|
|
||
| def self.load_project_config_file | ||
| load_config_file(config_file(".yard")) | ||
| end | ||
|
|
||
| def self.load_user_config_file | ||
| load_config_file(config_file(USER_CONFIG_DIR)) | ||
| end |
There was a problem hiding this comment.
These should be documented (and whichever are not intended to be used should be marked @private).
| before do | ||
| allow(File).to receive(:file?).with(".yard/config").and_return(false) | ||
| allow(File).to receive(:file?).with(".yardopts").and_return(false) | ||
| end | ||
|
|
There was a problem hiding this comment.
I would expect to see actual tests for the new APIs:
- load_config_file is missing tests.
- read_config_file behavior seems to be missing tests.
Some of this may already be tested via wider unit tests, but there should at least be a few scenarios for the theoretical project based usage.
|
Ooh yay, YARD isn't abandoned! I'm a little surprised at the pushback to the assumption that "project dir" == That said, this is one of my bigger PRs, and it's been a while since I've worked on it. I have several others - most of them I have not even sent as PRs, since I just started pointing my project at my own fork of YARD. If the others go well, I'll start sending more PR, and then consider putting in the time addressing the comments on this one. |
The issue isn't the assumption / default behavior, it's the lack of configurability. It's fine to assume the default, but YARD supports configuration of all of the other values, up to and including where the |
👍 ah okay, that makes more sense. I'll probably just re-implement as a switch to change the config dir, making sure that switch will be applied if it's in a yardopts file. That will be sufficient for my needs. |
so that you can configure a plugin per-project
Completed Tasks
bundle exec rakelocally (if code is attached to PR).