1200 feature GitHub action javascript linter formatter #1201
Conversation
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
Contributor
Reviewer's Guide by SourceryThis pull request introduces a GitHub action to lint and format JavaScript code. This addresses the current lack of quality controls for JavaScript code. Sequence diagram for authorization check flowsequenceDiagram
participant Client
participant AuthzWorker
participant CoreService
Client->>AuthzWorker: checkAuth(client_id, path, action)
AuthzWorker->>AuthzWorker: isURLValid(path)
AuthzWorker->>AuthzWorker: removeOrigin(path)
AuthzWorker->>AuthzWorker: isPathValid(path)
AuthzWorker->>AuthzWorker: isTestPath(path)
alt is test path
AuthzWorker-->>Client: return 0 (allowed)
else not test path
AuthzWorker->>CoreService: send authorization request
CoreService-->>AuthzWorker: response
AuthzWorker->>AuthzWorker: processResponse(response)
AuthzWorker-->>Client: return result
end
Class diagram for AuthzWorker refactoringclassDiagram
class AuthzWorker {
-Config* m_config
-LogContext m_log_context
-string m_local_globus_path_root
-string m_test_path
-map<CredentialType, string> m_cred_options
-unique_ptr<ICredentials> m_sec_ctx
-unique_ptr<ICommunicator> m_comm
+AuthzWorker(Config*, LogContext)
+checkAuth(char*, char*, char*) int
-isTestPath(string) bool
-isPathValid(string) bool
-isURLValid(char*) bool
-removeOrigin(char*) string
-getAuthzPath(char*) string
-processResponse(ICommunicator::Response&) int
-initCommunicator() void
}
note for AuthzWorker "Refactored class with improved
modularity and error handling"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…errors Address eslint errors
Collaborator
Author
|
Currently the formatter is passing and not failing because formatting needs to be fixed in a separate PR. JavaScript formatter is set to pass inspite of failures currently. |
AronPerez
suggested changes
Jan 2, 2025
Contributor
AronPerez
left a comment
AronPerez
left a comment
There was a problem hiding this comment.
actions/setup-node@v3 -> v4
actions/checkout@v3 -> v4
Linter + prettier config
| - name: Install ESLint | ||
| run: | | ||
| npm init -y | ||
| npm install eslint@latest @babel/eslint-parser@latest eslint-define-config globals --save |
Contributor
There was a problem hiding this comment.
Do we want to add a package.json to manage these scripts? That way we could just run npm run lint
Collaborator
Author
There was a problem hiding this comment.
This works as it is. What is the advantage of adding another file to the repo? I suppose if you want to test locally... and ensure the packages are the same?
| "vueIndentScriptAndStyle": false, | ||
| "proseWrap": "preserve", | ||
| "insertPragma": false, | ||
| "printWidth": 100, |
Collaborator
Author
There was a problem hiding this comment.
@t-ramz any particular reason, do you want to increase it?
Collaborator
There was a problem hiding this comment.
No reason, I am happy to increase it. In fact, I don't particularly like width limits at all. More is more here.
| "insertPragma": false, | ||
| "printWidth": 100, | ||
| "requirePragma": false, | ||
| "tabWidth": 4, |
Contributor
There was a problem hiding this comment.
Nit: I'm a fan of 2 tabs per tab width personally
Collaborator
Author
There was a problem hiding this comment.
Did you mean spaces? per tab? That is actually my default as well.
Collaborator
Author
There was a problem hiding this comment.
But I don't feel strongly about this.
| @@ -0,0 +1,21 @@ | |||
| { | |||
| "arrowParens": "always", | |||
| "singleQuote": false, | ||
| "jsxSingleQuote": false, | ||
| "quoteProps": "as-needed", | ||
| "trailingComma": "all", |
| continue-on-error: true # Allow this job to fail without failing the entire workflow | ||
| run: | | ||
| prettier "**/*.js" --write | ||
| git diff |
Collaborator
Author
There was a problem hiding this comment.
Prettier won't show the differences, it just shows what files will be changed.
Comment on lines
+4
to
+11
| module.exports = [{ | ||
| languageOptions: { | ||
| globals: { | ||
| ...globals.jquery, | ||
| ...globals.node, | ||
| }, | ||
| }, | ||
| }]; |
Contributor
There was a problem hiding this comment.
We should extend prettier to prevent rule collision issues:
Suggested change
| module.exports = [{ | |
| languageOptions: { | |
| globals: { | |
| ...globals.jquery, | |
| ...globals.node, | |
| }, | |
| }, | |
| }]; | |
| module.exports = [{ | |
| languageOptions: { | |
| globals: { | |
| ...globals.jquery, | |
| ...globals.node, | |
| }, | |
| }, | |
| plugins: ["prettier"], | |
| extends: ["plugin:prettier/recommended"], | |
| }]; |
Co-authored-by: Aaron Perez <aperez0295@gmail.com>
AronPerez
approved these changes
Jan 2, 2025
JoshuaSBrown
deleted the
1200-feature-github-action-javascript-linter-formatter
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Description
JavaScript code needs quality controls which are currently missing this is an attempt to add them.
Tasks
Summary by Sourcery
Add JavaScript linting and formatting using ESLint and Prettier.
CI:
Tests: