-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add configuration option requireConfig #43
Add configuration option requireConfig #43
Conversation
test/index.js
Outdated
const rcPath = new URL('.testremarkrc.json', import.meta.url) | ||
t.teardown(() => fs.rm(rcPath, {force: true})) | ||
await fs.writeFile(rcPath, '{}\n') | ||
const watchedFileDiagnosticsPromise = createOnNotificationPromise( | ||
connection, | ||
PublishDiagnosticsNotification.type | ||
) | ||
connection.sendNotification(DidChangeWatchedFilesNotification.type, { | ||
changes: [] | ||
}) | ||
const watchedFileDiagnostics = await watchedFileDiagnosticsPromise | ||
t.isNot( | ||
0, | ||
watchedFileDiagnostics.diagnostics.length, | ||
'should emit diagnostics if requireConfig is true with config' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t understand why this doesn’t work. The idea is that changing the rc configuration file retriggers validation, which is does, and then finds the configuration file, but it doesn’t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, with “but it doesn’t” you mean “which it doesn’t”, right? So the trigger works, then it can’t find the file again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is suuuuper long btw, really hard to get what’s happening (or supposed to happen) 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree it’s long. The other test is even longer. I tried grouping it using blank lines, but it’s still quite messy.
Basically 3 things happen:
- The server is started and initialized without workspace support.
- The file is opened. As a result file diagnostics are emitted (
diagnostics.length !== 0
) - The language server changes its option
requireConfig
totrue
..testremarkrc.json
is not present, so empty diagnostics are emitted. - The file
/home/remco/Projects/unified-language-server/test/.testremarkrc.json
file is created and the server is notified of this. This should result in diagnostics again, but the diagnostics are empty.
These are the values passed intounified-engine
(inspected using a debugger, then tweaked manually for representation here)The language server used is{ alwaysStringify: false, cwd: '/home/remco/Projects/unified-language-server', files: [ // VFile simplified for representation { messages: [], path: '/home/remco/Projects/unified-language-server/test/lsp.md', cwd: '/home/remco/Projects/unified-language-server', value: '# hi' } ], ignoreUnconfigured: true, plugins: [ './test/lots-of-warnings.js' ], quiet: false, rcName: 'testremark', silentlyIgnore: true, streamError: new PassThrough(), streamOut: new PassThrough() }
remark-with-warnings.js
:I really don’t understand why this yields empty diagnosticsimport {createUnifiedLanguageServer} from '../index.js' createUnifiedLanguageServer({ configurationSection: 'remark', processorName: 'remark', processorSpecifier: 'remark', rcName: 'testremark', // This is resolved from the directory containing package.json plugins: ['./test/lots-of-warnings.js'] })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this: step 2. removes the current file from the list of files, so if step 3. adds a config file, there are no files anymore in the corresponding folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits discovered by reading through the changes
This is a breaking change, but it’s ok since a major is about to be released.
This comment was marked as resolved.
This comment was marked as resolved.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 474 558 +84
=========================================
+ Hits 474 558 +84
☔ View full report in Codecov by Sentry. |
Can we merge this? There’s one open comment about the test. The tests are a bit long, but they test what they’re supposed to test. I would like to resolve some issues other that depend on these changes. Also this solves an issue a lot people run into, including myself. |
Initial checklist
Description of changes
Closes #30