Skip to content
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

[Feature] Add ESLint to the project #1383

Closed
wants to merge 15 commits into from
Closed

Conversation

rsilvr
Copy link
Contributor

@rsilvr rsilvr commented Jan 14, 2024

Description

  • Added ESLint config files and plugin as a dev dependency.
  • It should be available in most IDEs with the use of some plugin or as CLI running the command: eslint . in the desired directory.
  • I used the recommended preset and changed some rules from "error" to "warning" where I found to be too strict or inconvenient to the the current standards of the codebase.
  • The root file is configured to Node/Common.js projects and I overrode it in specific projects
  • I ran the plugin in every Bruno project and fixed some minor issues like unused imports and variables.
  • In some very few cases I found some undeclared variables that should be causing those lines to break (console.logs mostly).
  • Below some issues where I didn't make any changes but I think could better analyzed @helloanoop:

packages/bruno-electron/src/ipc/network/awsv4auth-helper.js

12:7 error 'credentialsProvider' is not defined no-undef
15:7 error 'credentials' is not defined no-undef
15:27 error 'credentialsProvider' is not defined no-undef
16:27 error 'credentials' is not defined no-undef
17:31 error 'credentials' is not defined no-undef
18:28 error 'credentials' is not defined no-undef

It seems that all those variables are being declared globally but maybe they could be declared as simple consts?

packages/bruno-app/src/utils/importers/postman-collection.js

25:34  error 'text' is not defined

packages/bruno-app/src/utils/common/index.js

56:12  error    'JSON5' is not defined

These seem like bugs

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Closes: #1382

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

@@ -133,11 +133,12 @@ export function get(source: any, path: string, ...fns: PredicateOrMapper[]) {
case token === '..':
case token === '.':
break;
case token === '?':
case token === '?': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -43,8 +43,11 @@ function MyApp({ Component, pageProps }) {

if (!window.ipcRenderer) {
return (
<div class="bg-red-100 border border-red-400 text-red-700 px-4 py-3 mx-10 my-10 rounded relative" role="alert">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

class -> className

@@ -45,7 +45,6 @@ export default class QueryEditor extends React.Component {
brunoVarInfo: {
variables: getAllVariables(this.props.collection)
},
theme: this.props.editorTheme || 'graphiql',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no editorTheme props being passed to this component on any of its occurrences

@rsilvr rsilvr marked this pull request as ready for review January 15, 2024 00:29
@rsilvr rsilvr closed this Feb 8, 2024
@rsilvr rsilvr reopened this Feb 8, 2024
@helloanoop
Copy link
Contributor

This is quite a significant change to the codebase, and given the current size of the PR backlog, I don't see us reviewing and merging this anytime soon.

We'll chart out an effort later to incrementally adopt a linter in the future.

@helloanoop helloanoop closed this Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] ESLint
3 participants