This repository was archived by the owner on Mar 5, 2026. It is now read-only.
Add options for ConfigFactory to handle customConfig properly#2588
Merged
mkondratek merged 2 commits intomainfrom Nov 7, 2024
Merged
Add options for ConfigFactory to handle customConfig properly#2588mkondratek merged 2 commits intomainfrom
mkondratek merged 2 commits intomainfrom
Conversation
mkondratek
commented
Nov 6, 2024
Comment on lines
+144
to
+142
| val config = ConfigFactory.parseString(text).resolve() | ||
| val config = JsonParser.parseString(text).asJsonObject |
Contributor
Author
There was a problem hiding this comment.
ConfigFactory does not render json. The rendered config contained the comment with # instead of //. Using JsonParser fixes the problem.
Contributor
There was a problem hiding this comment.
Other option would be adding .setComments(false) in the current code.
I thought Lightened Config was a bit more forgiving when parsing e.g. additional commas.
Maybe we can write simple test for that and check how those corner cases looks in case of both approaches?
mkondratek
force-pushed
the
mkondratek/fix/customConfigComments
branch
from
95ac480 to
6264807
Compare
mkondratek
changed the title
mkondratek
commented
Nov 7, 2024
| val config = ConfigFactory.parseString(text).resolve() | ||
| var config = ConfigFactory.parseString(text).resolve() | ||
| additionalProperties.forEach { (key, value) -> | ||
| config.withValue(key, ConfigValueFactory.fromAnyRef(value)) |
Contributor
Author
There was a problem hiding this comment.
There was a bug 😄 configs are immutable
Contributor
Author
There was a problem hiding this comment.
Basically, we ignored "foldingRanges": "indentation-based" - how important for us is this option?
mkondratek
commented
Nov 7, 2024
Comment on lines
+27
to
+33
| parsed | ||
| .get("cody") | ||
| .asJsonObject | ||
| .get("experimental") | ||
| .asJsonObject | ||
| .get("foldingRanges") | ||
| .asString) |
Contributor
Author
There was a problem hiding this comment.
withValue adds the field as
"cody": {
"experimental": {
"foldingRanges": "indentation-based"
}
}
instead of
"cody.experimental.foldingRanges": "indentation-based"
Is it a problem? 🤔
Contributor
There was a problem hiding this comment.
IMHO It is not, those are equivalent in VSC config.
kalanchan
pushed a commit
that referenced
this pull request
Nov 8, 2024
Fixes https://linear.app/sourcegraph/issue/CODY-4264/a-comment-in-cody-settingsjson-breaks-cody-initialization-stuck-at. ## Test plan 1. Have the cody_settings.json like this: ``` { // other providers... "openctx.providers": { "https://gist.githubusercontent.com/thenamankumar/3708f084fb2abd57adafe7f14620bdf7/raw/e7c7059cb5b04f6feead002e7b3a90c5b1a4bb96/provider.js": true } } ``` 2. Cody should initialize
kalanchan
added a commit
that referenced
this pull request
Nov 8, 2024
cherry pick #2588 - allowing more flexibility as we didn't inform the team when branch cut was Fixes https://linear.app/sourcegraph/issue/CODY-4264/a-comment-in-cody-settingsjson-breaks-cody-initialization-stuck-at. ## Test plan 1. Have the cody_settings.json like this: ``` { // other providers... "openctx.providers": { "https://gist.githubusercontent.com/thenamankumar/3708f084fb2abd57adafe7f14620bdf7/raw/e7c7059cb5b04f6feead002e7b3a90c5b1a4bb96/provider.js": true } } ``` 2. Cody should initialize ## Test plan <!-- All pull requests REQUIRE a test plan: https://sourcegraph.com/docs/dev/background-information/testing_principles Why does it matter? These test plans are there to demonstrate that are following industry standards which are important or critical for our customers. They might be read by customers or an auditor. There are meant be simple and easy to read. Simply explain what you did to ensure your changes are correct! Here are a non exhaustive list of test plan examples to help you: - Making changes on a given feature or component: - "Covered by existing tests" or "CI" for the shortest possible plan if there is zero ambiguity - "Added new tests" - "Manually tested" (if non trivial, share some output, logs, or screenshot) - Updating docs: - "previewed locally" - share a screenshot if you want to be thorough - Updating deps, that would typically fail immediately in CI if incorrect - "CI" - "locally tested" --> Co-authored-by: Mikołaj Kondratek <mik.kondratek@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes https://linear.app/sourcegraph/issue/CODY-4264/a-comment-in-cody-settingsjson-breaks-cody-initialization-stuck-at.
Test plan