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

feat(vscode): watch configuration file #3284

Merged
merged 9 commits into from
Nov 2, 2022
Merged

Conversation

ematipico
Copy link
Contributor

Summary

This PR adds a new feature to the VSCode extension. It allows to watch changes to the rome.json file and when new changes occur, a listener sends a request. The server will listen to this request and will update the workspace settings.

I also added and change some logs, and used the debug trace instead. Our filter used the DEBUG level.

Test Plan

I manually tested the feature

@ematipico ematipico requested a review from leops as a code owner September 27, 2022 13:30
@ematipico ematipico requested a review from a team September 27, 2022 13:30
@netlify
Copy link

netlify bot commented Sep 27, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 148526a
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/636284e527bb4f0008c4d263

@ematipico ematipico force-pushed the feature/file-change-detection branch from e459d8c to c6b2238 Compare September 27, 2022 13:40
@ematipico ematipico marked this pull request as draft September 27, 2022 13:48
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

My main concern is around naming where there is a mix between "Settings" and configuration

@ematipico
Copy link
Contributor Author

ematipico commented Sep 28, 2022

My main concern is around naming where there is a mix between "Settings" and configuration

I can see the confusion that settings and configuration can cause, and I think it could make sense to normalize the naming convention. Although, these two different names allow us to create a distinction:

  • the concept of configuration belongs to the users: user-friendly data structure, data structure we need to serialize/deserialize
  • the concept of settings belongs to us developers - and the Workspace, where the configuration is actually transformed into another data structure that fits the needs of Rome's infrastructure. An example is the "globals": [""] field, which is transformed into a Matcher data structure

If we decided to normalize the naming convention, we might risk to lose this distinction. What do you think? How can we improve this division?

@ematipico ematipico temporarily deployed to netlify-playground September 28, 2022 10:41 Inactive
@github-actions
Copy link

github-actions bot commented Sep 28, 2022

@ematipico ematipico marked this pull request as ready for review September 28, 2022 12:14
@ematipico ematipico added A-Editors Area: editors E-VScode Editors: VSCode labels Sep 30, 2022
@ematipico ematipico added this to the 0.10.0 milestone Sep 30, 2022
@MichaReiser
Copy link
Contributor

My main concern is around naming where there is a mix between "Settings" and configuration

I can see the confusion that settings and configuration can cause, and I think it could make sense to normalize the naming convention. Although, these two different names allow us to create a distinction:

* the concept of configuration belongs to the users: user-friendly data structure,  data structure we need to serialize/deserialize

* the concept of settings belongs to us developers - and the `Workspace`, where the configuration is actually **transformed** into another data structure that fits the needs of Rome's infrastructure. An example is the `"globals": [""]` field, which is transformed into a `Matcher` data structure

If we decided to normalize the naming convention, we might risk to lose this distinction. What do you think? How can we improve this division?

Oh, I wasn't aware that these are two different things. It may then make sense to keep the distinct names.

@ematipico ematipico modified the milestones: 0.10.0, 10.0.0 Oct 3, 2022
@ematipico ematipico added the PR: on hold A PR that needs some upstream work before getting merged. label Oct 3, 2022
@ematipico ematipico force-pushed the feature/file-change-detection branch from 76acd87 to 91c44ba Compare November 1, 2022 15:00
@ematipico ematipico force-pushed the feature/file-change-detection branch from 91c44ba to 369123f Compare November 1, 2022 15:04
@ematipico ematipico removed the PR: on hold A PR that needs some upstream work before getting merged. label Nov 1, 2022
@ematipico ematipico force-pushed the feature/file-change-detection branch from 369123f to 4a47d64 Compare November 1, 2022 15:06
@ematipico ematipico temporarily deployed to netlify-playground November 1, 2022 15:10 Inactive
@ematipico ematipico force-pushed the feature/file-change-detection branch from e785025 to 3c761b7 Compare November 1, 2022 16:34
@ematipico ematipico temporarily deployed to netlify-playground November 1, 2022 16:34 Inactive
@ematipico ematipico mentioned this pull request Nov 1, 2022
@ematipico ematipico temporarily deployed to netlify-playground November 1, 2022 16:42 Inactive
@ematipico ematipico requested a review from MichaReiser November 1, 2022 16:56
@ematipico ematipico temporarily deployed to netlify-playground November 2, 2022 14:05 Inactive
@ematipico ematipico requested a review from leops November 2, 2022 14:05
@ematipico
Copy link
Contributor Author

@leops that's working now, using the server instead of the client

@ematipico ematipico temporarily deployed to netlify-playground November 2, 2022 14:55 Inactive
@ematipico ematipico merged commit 23ba403 into main Nov 2, 2022
@ematipico ematipico deleted the feature/file-change-detection branch November 2, 2022 15:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Editors Area: editors E-VScode Editors: VSCode
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载