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

refactor(docs): apply eslint rules to code snippets #1259

Merged
merged 3 commits into from
Feb 7, 2023

Conversation

niko-achilles
Copy link
Contributor

@niko-achilles niko-achilles commented Jan 21, 2023

Description of your changes

  1. introduces : root: true in eslint configuration file in root of repository which manages the workspace.
  2. applies existing eslint rules defined in eslint configuration file in root of repository
  3. fixes code snippets in docs/snippets lintinting issues which are related to styling: semi, quotes, indent
  4. introduces overrides section in eslint configuration file in root of repository, so that we can discuss explicit about typescript linting issues for code snippets: no-explicit-any, no-unused-vars, explicit-member-accessibility, no-var-requires ,
  5. overrides section in eslint configuration file in root of repository, has no impact or side effects to the overall code base of this repository.
  6. missing dependency of @aws-sdk/client-s3 defined as dev. dependency in package.json of docs/snippets

TODOs:
options / trade-offs, typescript linting issues for code snippets:

  • no-explicit-any, no-unused-vars, explicit-member-accessibility, no-var-requires

How to verify this change

  1. Verify that root eslint configuration file is used by running: npm run -w docs/snippets lint -- --debug
    should yield
...
 eslintrc:config-array-factory Loading JS config file: /aws-lambda-powertools-typescript/.eslintrc.js +1ms
...
eslintrc:cascading-config-array-factory Stop traversing because of 'root:true'. +806ms

  1. Run: npm run -w docs/snippets lint

Related issues, RFCs

Issue number: #1252 #729

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pull-request-size pull-request-size bot added the size/XL PRs between 500-999 LOC, often PRs that grown with feedback label Jan 21, 2023
@niko-achilles
Copy link
Contributor Author

Please review so that we can discuss about the TODOs as described in this PR comment:
options / trade-offs, typescript linting issues for code snippets.

  • no-explicit-any, no-unused-vars, explicit-member-accessibility, no-var-requires

// cc. @dreamorosi

@github-actions github-actions bot added the bug Something isn't working label Jan 22, 2023
@saragerion saragerion self-requested a review January 23, 2023 13:50
@dreamorosi dreamorosi changed the title fix(docs): apply eslint rules to code snippets refactor(docs): apply eslint rules to code snippets Jan 24, 2023
@dreamorosi dreamorosi added internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) and removed bug Something isn't working labels Jan 24, 2023
@dreamorosi dreamorosi linked an issue Jan 24, 2023 that may be closed by this pull request
2 tasks
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Jan 24, 2023
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

I've added a number of comments.

I think we are going in the right direction, maybe one/two more iterations and we'll get there.

Aside from the comments I have left, please also change the .github/workflows/reusable-run-linting-check-and-unit-tests.yml file to add -w docs/snippets:

Thanks!

docs/snippets/package.json Outdated Show resolved Hide resolved
docs/snippets/tracer/captureHTTP.ts Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
docs/snippets/logger/basicUsage.ts Outdated Show resolved Hide resolved
docs/snippets/logger/unitTesting.ts Outdated Show resolved Hide resolved
docs/snippets/tracer/disableCaptureResponseHandler.ts Outdated Show resolved Hide resolved
docs/snippets/tracer/putMetadata.ts Outdated Show resolved Hide resolved
docs/snippets/tracer/putMetadata.ts Outdated Show resolved Hide resolved
docs/snippets/tracer/middy.ts Outdated Show resolved Hide resolved
docs/snippets/tracer/manual.ts Outdated Show resolved Hide resolved
@niko-achilles
Copy link
Contributor Author

niko-achilles commented Jan 26, 2023

@dreamorosi proposals applied with additional new commit.
Please review and we can discuss to apply further changes if needed.

Note: docs/snippets/tracer/captureAWSAll.ts needs special treatment .

@niko-achilles niko-achilles requested review from dreamorosi and removed request for saragerion January 26, 2023 00:10
saragerion
saragerion previously approved these changes Jan 27, 2023
Copy link
Contributor

@saragerion saragerion left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for opening this PR

.eslintrc.js Outdated Show resolved Hide resolved
@niko-achilles
Copy link
Contributor Author

LGTM! Thanks for opening this PR

@saragerion i think the docs/snippets/tracer/captureAWSAll.ts needs special treatment and guide from you or other reviewers as maintainers of this repository.

What do you think ?

@dreamorosi having background information from the beginning of this journey with extracting and linting code snippets
may give guidance also.

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Hi Niko, I'm now back at work and I was able to look at the captureAWSAll code snippet.

I have proposed an alternative that works and shouldn't require any special override to the linting rules.

Whenever you can please apply this suggestion and resolve the merge conflicts. Looking forward to merge this and move on.

docs/snippets/tracer/captureAWSAll.ts Show resolved Hide resolved
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thank you for the work on this, I appreciate the effort you've put in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature PRs that introduce new features or minor changes internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) size/XL PRs between 500-999 LOC, often PRs that grown with feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: apply linting to docs/snippets code snippets
3 participants