- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5
[Feature] Implement diagnostic and quick fix for missing configuration #451
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
base: development
Are you sure you want to change the base?
[Feature] Implement diagnostic and quick fix for missing configuration #451
Conversation
| 
 | 
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've noticed that our .pluplugin directory handling is a bit inaccurate in case the user opens multiple workspace folders. See #454 as a potential follow up on this.
        
          
                packages/language/src/language-server/code-actions/apply-quick-fixes.ts
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                packages/language/src/language-server/code-actions/apply-quick-fixes.ts
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                packages/language/src/language-server/code-actions/apply-quick-fixes.ts
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                packages/language/src/workspace/plugin-configuration-provider.ts
              
                Outdated
          
            Show resolved
            Hide resolved
        
      0b0ee24    to
    6cd2b92      
    Compare
  
    … Libs bug for inexistent lib. Signed-off-by: Wagner Laranjeiras <[email protected]>
ceffbcf    to
    f71af15      
    Compare
  
    Signed-off-by: Wagner Laranjeiras <[email protected]>
| }; | ||
| let diagnostic: Diagnostic; | ||
|  | ||
| if (!(await hasFullConfig())) { | 
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.
Suggestion: I don't think we necessarily need to wait here for the file system. Assuming the current unit has a config available to it, it will be accessible via the processGroup/programConfig field. Accessing the file system might considerably slow down the interpreter in the error case.
| diagnostic = diagnosticFromCode( | ||
| InternalCodes.DiagnosticMissingConfiguration, | ||
| item.token, | ||
| ); | ||
| diagnostic.data = { entryUri: context.entryUri.toString() }; | 
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.
Suggestion: This shouldn't be implemented as an internal code. Internal codes are internal, because they shouldn't be displayed to the user. Generating this diagnostic however will show the internal code to the user. Reuse the IBM3841I code instead and add additional data into the .data field to differentiate between the quick fixes that are applied to these diagnostics.
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.
Ah ok. I understood that Internal codes were custom ones we had generate outside of the PLI-Codes.
| DiagnosticMissingConfiguration: { | ||
| code: "_TB002", | ||
| severity: Severity.E, | ||
| message: "Configuration is missing.", | ||
| fullCode: "_TB002E", | ||
| } as SimplePLICode, | 
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.
Please remove, see reason above.
| // add any contained directories to the libs list, as well as the toProcess list | ||
| const libUri = UriUtils.joinPath(URI.parse(this.workspacePath), lib); | ||
| const entries = await FileSystemProviderInstance.readDir(libUri); | ||
| console.log(entries); | 
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.
Please remove before undrafting this PR.
| try { | ||
| // directory to add for handling | ||
| libsToProcess.push(`${lib}/${fileName}`); | ||
| // also add to the full libs list | ||
| computedLibs.add(`${lib}/${fileName}`); | ||
| } catch (err) { | ||
| console.error(`Failed to add ${lib}/${fileName}: `, err); | ||
| } | 
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.
Question: How can an array push/a set add operation fail?
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 was testing if somehow that could be the problem and ended up letting this there because I wasn't sure it couldn't fail. Gonna remove it though.
| } catch (err) { | ||
| console.error(`Error reading directory: ${uri.fsPath}`, err); | ||
| return []; | ||
| } | 
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.
Suggestion: IMO the call site should handle any file system failure. Only the caller knows what to do with a failure:
- Do nothing (which would be essentially the same as returning []).
- Log the error and continue (perhaps displaying it to the user)
- Bubble up the error
Signed-off-by: Wagner Laranjeiras <[email protected]>
…ix-for-missing-config Signed-off-by: Wagner-Laranjeiras <[email protected]>
Signed-off-by: Wagner Laranjeiras <[email protected]>
Signed-off-by: Wagner Laranjeiras <[email protected]>
Signed-off-by: Wagner Laranjeiras <[email protected]>
Signed-off-by: Wagner Laranjeiras <[email protected]>
Signed-off-by: Wagner Laranjeiras <[email protected]>
| import { Severity } from "../language-server/types"; | ||
| import { SimplePLICode } from "../validation/pli-codes"; | ||
|  | ||
| export const lspCodes = { | 
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.
Minor: Exported constants are usually either CamelCase or SNAKE_CASE.
| export const lspCodes = { | |
| export const LspCodes = { | 
| /** | ||
| * This program and the accompanying materials are made available under the terms of the | ||
| * Eclipse Public License v2.0 which accompanies this distribution, and is available at | ||
| * https://www.eclipse.org/legal/epl-v20.html | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Copyright Contributors to the Zowe Project. | ||
| * | ||
| */ | ||
|  | ||
| import { Severity } from "../language-server/types"; | ||
| import { SimplePLICode } from "../validation/pli-codes"; | 
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.
Suggestion: Please put this file in the validation part of the package.
[Feature] Implement diagnostic and quick fix for missing configuration and [Bugfix] Fix non existent lib breaking all INCLUDES.
Issue: XXX
Summary:
[Feature] This PR introduces a new Diagnostic (_TB002) and Quick Fix action (pli.applyQuickFixCreateConfig) for missing files. When the config files are not present, the diagnostic will be thrown at the file in the %INCLUDE. The Quick Fix will then offer creating the configuration, having the current file as the program entry point.
[Bugfix] Fix INCLUDES resolution breaking because of non existent directory added to libs.
Changes
Notes