Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_service): traverse upwards the fs to discover the config file #4224

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

ematipico
Copy link
Contributor

Summary

This PR adds a way to discover rome.json by traversing the file system upwards.

Test Plan

I manually tested the feature using the CLI in our internal repository, and tested manually the VSCode extension.

Below a recording to demonstrate the feature:

Screen.Recording.2023-02-21.at.11.57.24.mov

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Feb 21, 2023

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit e91314c
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/63f623f5e21d610007404858

@ematipico
Copy link
Contributor Author

CI is failing on ubuntu... it seems the some code hits an infinite loop

@realtimetodie
Copy link
Contributor

I like this idea and I think this is useful, nonetheless I would appreciate it if this could become an optional opt-in feature, for example with a flag --auto-search. Traversing upwards breaks sandboxing in Bazel. This is a problem. For example, a rome.json configuration file in a parent directory would create false positives.

let config_name = file_system.config_name();
let configuration_path = match base_path {
BasePath::Lsp(ref path) | BasePath::FromUser(ref path) => path.join(config_name),
let mut configuration_path = match base_path {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful for users, if we could show a message when a rome.json configuration file was found using upwards-traversal (auto-search).

Alternatively, we could always show the path of the current rome.json configuration file (if any).

The from_parent variable is a bool primitive.

Suggested change
let mut configuration_path = match base_path {
let mut from_parent = false;
let mut configuration_path = match base_path {

@ematipico
Copy link
Contributor Author

ematipico commented Feb 21, 2023

I like this idea and I think this is useful, nonetheless I would appreciate it if this could become an optional opt-in feature, for example with a flag --auto-search. Traversing upwards breaks sandboxing in Bazel. This is a problem. For example, a rome.json configuration file in a parent directory would create false positives.

Are you open to having the feature opt-out, instead? I made the feature like this because it's used by the LSP too, and this is a feature requested by many users. And, as far as I understood, the prettier/eslint VSCode extensions do the same.

What do you think?

@realtimetodie
Copy link
Contributor

Yes of course, opt-out is fine. In Bazel, the path to the rome.json configuration path is set explicitly using the new --config-path option.

@realtimetodie
Copy link
Contributor

realtimetodie commented Feb 21, 2023

Maybe auto-traversal could be disabled, when the --config-path option is used (ConfigurationBasePath::FromUser(PathBuf)). Then we would not need a separate opt-out option?

@ematipico
Copy link
Contributor Author

Maybe auto-traversal could be disabled, when the --config-path option is used (ConfigurationBasePath::FromUser(PathBuf)). Then we would not need a separate opt-out option?

That's a good suggestion! I like it!

@ematipico ematipico added this pull request to the merge queue Feb 23, 2023
Merged via the queue into main with commit 582d078 Feb 23, 2023
@ematipico ematipico deleted the feat/parent-configuration branch February 23, 2023 09:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants