powertools-lambda-typescript icon indicating copy to clipboard operation
powertools-lambda-typescript copied to clipboard

Documentation (all): Extract code examples into separate files for linting

Open michaelbrewer opened this issue 3 years ago • 13 comments

Description of the improvement

Summary of the proposal

Code examples should be extracted to allow for validation / linting to help prevent errors.

How, where did you look for information

See

  • https://github.com/awslabs/aws-lambda-powertools-python/issues/1009
  • https://github.com/awslabs/aws-lambda-powertools-python/issues/1064

Missing or unclear documentation

Improvement

  • Extract examples into the docs/ folder, to allow for mkdocs to automatically pick it up
  • Add autoformatter and linting against these examples
  • See python pr as an example: https://github.com/awslabs/aws-lambda-powertools-python/pull/1065

Related existing documentation

Related issues, RFCs

Example of documentation examples with errors in them:

  • https://github.com/awslabs/aws-lambda-powertools-typescript/pull/472

michaelbrewer avatar Apr 05 '22 17:04 michaelbrewer

Hi Michael, first off thanks for opening an issue to discuss the proposal.

I think this idea is valuable, I got bitten by this a few times already and having the same linting rules applied to the code snippets would help.

I have a few questions though, mainly coming from my limited knowledge of MKDocs:

  1. If we separate the files can we still see them in context when authoring the docs locally?
  2. Would each code snippet have its own file?
  3. Would it be possible to use the same exact linting rules that we use for the rest of the project?
  4. When would this linting step run?

If the answer to number 2 is yes, then if we move forward with the idea, I'd prefer to have a separate PR for each utility: i.e. one PR that addresses all the examples in docs/core/metrics, another for docs/core/logger, etc. This way we can keep the PR size small and relatively easy to review.

I've marked this as low priority because at the moment we are focusing on the other issues marked as high before we get to GA.

With that said, after the points above have been discussed and agreed upon I'm open to review a PR from the community.

dreamorosi avatar Apr 05 '22 19:04 dreamorosi

Hi Michael, first off thanks for opening an issue to discuss the proposal.

I think this idea is valuable, I got bitten by this a few times already and having the same linting rules applied to the code snippets would help.

I have a few questions though, mainly coming from my limited knowledge of MKDocs:

  1. If we separate the files can we still see them in context when authoring the docs locally?
  2. Would each code snippet have its own file?
  3. Would it be possible to use the same exact linting rules that we use for the rest of the project?
  4. When would this linting step run?

If the answer to number 2 is yes, then if we move forward with the idea, I'd prefer to have a separate PR for each utility: i.e. one PR that addresses all the examples in docs/core/metrics, another for docs/core/logger, etc. This way we can keep the PR size small and relatively easy to review.

I've marked this as low priority because at the moment we are focusing on the other issues marked as high before we get to GA.

With that said, after the points above have been discussed and agreed upon I'm open to review a PR from the community.

It would be option 2, each example would have it’s own file. You might have shared examples, but a PR per page makes sense.

michaelbrewer avatar Apr 05 '22 19:04 michaelbrewer

@dreamorosi example PR is up #730

michaelbrewer avatar Apr 05 '22 21:04 michaelbrewer

Thanks for the PR, but I still would like to clear all the other points before start to review:

With that said, after the points above have been discussed and agreed upon I'm open to review a PR from the community.

Specifically I'd like to understand how/when we are going to actually lint these files.

At the moment I don't have yet a clear picture aside from just splitting the files and if there's no linting then having multiple files just makes it harder to maintain.

dreamorosi avatar Apr 05 '22 21:04 dreamorosi

Linting would be hard if you are doing something too strict (like unused variables), but at least it should handle syntax errors, missing parameters etc..

michaelbrewer avatar Apr 05 '22 22:04 michaelbrewer

@dreamorosi so far, i have already found a couple issues like the indents and the same yaml not being valid.

One option could be to configure a more lenient tsconfig to focus on compile errors. However you might still run into errors like:

Screen Shot 2022-04-06 at 8 49 30 AM

michaelbrewer avatar Apr 06 '22 15:04 michaelbrewer

Thanks for looking into it. I'd like to hear the opinion of the other maintainers as well since it seems that there's no clear-cut way and I'm not clear on whether the overhead of having many more files is worth the gain.

Finally as I have mentioned in my first comment, this is a low priority ticket so please expect some delays in the responses.

dreamorosi avatar Apr 06 '22 16:04 dreamorosi

Sure, i get the delay, maybe discuss with the maintainers: What is the objectives of the code examples? Should they be runnable or only valid syntax? It is easier to open and edit an example in VSCode with a .ts extension or just inline edit within the .md files?

The part that is frustrating to me is when an example does not have a valid syntax, or in the case of YAML not linted and has invalid indents.

michaelbrewer avatar Apr 06 '22 16:04 michaelbrewer

Thanks for opening this issue @michaelbrewer. This is an improvement that can potentially make sure our docs have better linted examples (which is a very good thing!).

The team is currently busy with other units of work with higher priority for the short term (GA), and we'll discuss about this and get back to you once we have the padding.

saragerion avatar Apr 06 '22 21:04 saragerion

Sure thing already run a couple more issues in the docs.

So i can either file issues for them or keep it fixed in the 1 PR #730

Bugs so far for Tracer docs:

  • fix indents to match the projects code style of 2 spaces (minor)
  • fix the sam template.yml example, by adding missing fields (handler, AWSTemplateFormatVersion etc.)
  • fix missing import for Context tracing https requests example

michaelbrewer avatar Apr 06 '22 22:04 michaelbrewer

Hi @michaelbrewer, I see value in the idea behind the proposal (extract the code snippet so we can apply some validation to them) and I agree with Sara, but at the moment we are extracting the files but I'm not seeing how do we actually lint/validate them.

In a previous comment you mentioned that there could be an option to set up some lenient tsconfig:

One option could be to configure a more lenient tsconfig to focus on compile errors.

If you're still interested in this, do you think you could make a proposal on how we should apply such linting? With that included in the PR we can see the results and close the circle (and so review/merge it).

dreamorosi avatar Apr 14 '22 12:04 dreamorosi

@dreamorosi I do have something for this locally, it will mean some small changes in the examples and for the aws-sdk to also be installed. I will see if i can share it.

michaelbrewer avatar Apr 14 '22 16:04 michaelbrewer

Sure, the aws-sdk is already a dev dependency so it won't be an issue. Thanks for your time!

dreamorosi avatar Apr 14 '22 22:04 dreamorosi

The issue is still current and a similar effort has been made in the Python's version of this library which might serve as inspiration.

Before taking this on there are some points to discuss:

  • Some of the examples show only pieces of code so they wouldn't necessarily compile, how to handle these?
  • How should these code snippet be linted?
  • At which point of the process should linting happen? (i.e. before merging a PR)

If anyone is interested in picking this up, please leave a comment here, then discuss your findings and proposed changes before opening a PR.

dreamorosi avatar Nov 09 '22 15:11 dreamorosi

@dreamorosi i can pick up this issue .

niko-achilles avatar Dec 24 '22 10:12 niko-achilles

Hi @niko-achilles, thanks for stepping up on this.

As I mentioned in the previous comment before opening a PR, let's discuss a plan of how this could be implemented under this issue.

If you have any question or need any input, I'm happy to help.

dreamorosi avatar Dec 24 '22 10:12 dreamorosi

@dreamorosi reading at the comments , are the following points the scope of this issue ?

  1. Some of the examples show only pieces of code so they wouldn't necessarily compile, how to handle these?
  2. How should these code snippet be linted?
  3. At which point of the process should linting happen? (i.e. before merging a PR)

As first thought Points 1,2 are independent from Point 3 .

For 1,2 i will introduce approaches to discuss . For 3 i will need further input .

niko-achilles avatar Dec 24 '22 10:12 niko-achilles

Agreed, let's start with the first two points and then once we have a clearer idea I'll propose where/when to add this step to the CI/CD.

dreamorosi avatar Dec 24 '22 10:12 dreamorosi

@dreamorosi , I had a hands-on experience with the aws-lambda-powertools-typescript repository the last 4 days and I have some minor recommendations to improve the developer usability for linting. It is about the .eslintrc.js file , which is used for linting typescript files, see Minor Recommendations below.

Furthermore i completed the research for the following:

  • linting tsdoc / jsdoc code blocks,
  • extending ESLint configuration Files to support linting typescript / javascript code snippets
  • cloudformation linting and cloudformation linting with serverless rule set
  • linting markdown ,
  • spell check,
  • generating api documentation with typedoc and
  • balancing, having in sync marketing documentation generated with static site generators like mkdocs with api documentation generated with typedoc.

So the next step would be to bring the approaches for discussion.

Minor Recommendations:

As is: https://github.com/awslabs/aws-lambda-powertools-typescript/blob/a09e4dfbb2cef062c1178de3e3dbc2583aef7a91/.eslintrc.js#L21

As it could be, see section Rule Removals typescript-eslint release: https://github.com/typescript-eslint/typescript-eslint/releases/tag/v3.0.0

and see new Rule ban-ts-comment: https://typescript-eslint.io/rules/ban-ts-comment/

and note that new Rule ban-ts-comment is included in recommended and by default ts-ignore is allowed

so it could be switched off.

"@typescript-eslint/ban-ts-comment": "off"

As is: https://github.com/awslabs/aws-lambda-powertools-typescript/blob/a09e4dfbb2cef062c1178de3e3dbc2583aef7a91/.eslintrc.js#L22

As it could be, see section Rule Removals typescript-eslint release: https://github.com/typescript-eslint/typescript-eslint/releases/tag/v3.0.0

and see new Rule naming convention: https://typescript-eslint.io/rules/naming-convention

and note that new Rule naming-convention when used requires type information, however is not included in recommended.

So @typescript-eslint/camelcase can be removed from eslintrc.js file

As is: https://github.com/awslabs/aws-lambda-powertools-typescript/blob/a09e4dfbb2cef062c1178de3e3dbc2583aef7a91/.eslintrc.js#L26

As it could be: same recommendation as 3.

So @typescript-eslint/interface-name-prefix could be removed from eslintrc.js file

Additional Minor Recommendation:

reason of improving is:

  • remove duplication of configuration values, one less configuration

As is: https://github.com/awslabs/aws-lambda-powertools-typescript/blob/a09e4dfbb2cef062c1178de3e3dbc2583aef7a91/.eslintrc.js#L8

As it could be, see section Config of typescript-eslint release: https://github.com/typescript-eslint/typescript-eslint/releases/tag/v3.0.0

{
  ...
  "extends": [
    "eslint:recommended",
    "plugin:@typescript-eslint/recommended"
  ],
  ...
}

niko-achilles avatar Dec 29 '22 00:12 niko-achilles

Exercising the codebase in context of linting and taking under consideration my my last minor additional recommendation about eslint:recommended, plugin:@typescript-eslint/recommended that would be a major change in dev. experience .

{
  ...
  "extends": [
    "eslint:recommended",
    "plugin:@typescript-eslint/recommended"
  ],
  ...
}

Reasoning: Eslint throws additional errors defined in eslint:recommended. However they could be important to consider (especially the Unnecessary catch clause error report). Additonal argument could be that is a good practice to follow and subscribe to the releases of eslint and typescript-eslint , so that the linting configuration can improve / adjusted iteratively. E.g:

See below.

/workspace/aws-lambda-powertools-typescript/packages/commons/tests/unit/LambdaInterface.test.ts
  88:13  error  Unnecessary catch clause  no-useless-catch

/workspace/aws-lambda-powertools-typescript/packages/logger/src/Logger.ts
  338:11  error  Unnecessary catch clause                                                   no-useless-catch
  359:28  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins

/workspace/aws-lambda-powertools-typescript/packages/metrics/src/Metrics.ts
  278:11  error  Unnecessary catch clause  no-useless-catch

/workspace/aws-lambda-powertools-typescript/packages/tracer/tests/e2e/allFeatures.decorator.test.ts
  320:34  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins

/workspace/aws-lambda-powertools-typescript/packages/tracer/tests/e2e/allFeatures.middy.test.functionCode.ts
  29:3  error  Unnecessary try/catch wrapper  no-useless-catch

/workspace/aws-lambda-powertools-typescript/packages/tracer/tests/e2e/allFeatures.middy.test.ts
  316:34  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins

/workspace/aws-lambda-powertools-typescript/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts
  36:5  error  Unnecessary try/catch wrapper  no-useless-catch

/workspace/aws-lambda-powertools-typescript/packages/tracer/tests/helpers/traceAssertions.ts
  32:28  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins

✖ 9 problems (9 errors, 0 warnings)

niko-achilles avatar Dec 30 '22 00:12 niko-achilles

Hi @niko-achilles, thank you for looking into this.

After reading your two previous comments I'm not sure we should focus on changing the eslint configuration for the whole project, especially if this will require significant changes in the codebase.

While I understand that some of the rules might appear redundant/overlap with the recommended preset, it also seems that the preset comes with a series of other rules that we might now want.

In an effort to refocus the activity, and to clarify the scope, I think first and foremost we should focus on finding a way of linting only the code present in the documentation. We can address other types of linting / spellchecks / etc. at a later time.

dreamorosi avatar Dec 30 '22 01:12 dreamorosi

@dreamorosi ok, you are right about the focus for introducing the approaches of linting only the code present in the documentation. In the approaches a good linting configuration is essential, so i read and played with all of the linting configurations in the repo 😋

Only the second comment above can have impact on 9 code lines, if eslint recommended is declared in the configs and the 2 rules mentioned are switched on . If switched off no impact in released code .

I have found also a duplicate rule in the linting configurations (minor - nothing special)

Duplicate rules - '@typescript-eslint/semi': [ 'error', 'always' ], - semi: [ 'error', 'always' ]

which yield when linting and forgetting to put semicolon

37:19  error  Missing semicolon  @typescript-eslint/semi
37:19  error  Missing semicolon  semi

can be fixed as:

  • '@typescript-eslint/semi': [ 'error', 'always' ],
  • semi: 'off'

niko-achilles avatar Dec 30 '22 03:12 niko-achilles

Hi Niko, I have assigned the issue to you for the time being since you've been putting a lot of work into it.

I agree that good linting configuration is important, but at the moment I'm not seeing how the changes will help with the task at hand. If these changes are necessary to make possible the linting of the code snippets in the docs I'm open to discuss them as part of this issue, otherwise maybe we should open a separate issue and discuss/prioritize it accordingly.

If these changes are needed for the docs linting, I'd like also to better understand why/how the current config is not compatible with it.

Again, in principle I think the changes proposed are an improvement, but I want to make sure I understand the motivation behind them and the implications. This will help me understand how they fit with the rest of the project and its direction.

dreamorosi avatar Dec 30 '22 09:12 dreamorosi

Hi Niko, I have assigned the issue to you for the time being since you've been putting a lot of work into it.

Its my pleasure to take this issue . @dreamorosi with the guidance of you and powertools maintainers, I take the responsibility , invest time and provide solutions that make the contributors of powertools happy.

I agree that good linting configuration is important, but at the moment I'm not seeing how the changes will help with the task at hand. If these changes are necessary to make possible the linting of the code snippets in the docs I'm open to discuss them as part of this issue, otherwise maybe we should open a separate issue and discuss/prioritize it accordingly.

@dreamorosi The recommendations mentioned above ref1, ref2, ref3

are minor improvements of the rules defined in the eslint configurations. I would recommend to open a new issue and set the focus of the new issue on Eslint Rules improvement .

@dreamorosi If you agree i can create a new issue and also take the task to provide solution as Assignee .

Again, in principle I think the changes proposed are an improvement, but I want to make sure I understand the motivation behind them and the implications. This will help me understand how they fit with the rest of the project and its direction.

@dreamorosi The motivation behind my recommendations

ref1, ref2, ref3

is to make aware that the linting configuration as is today has some issues that make me thinking that the causality relies on missing process or approach for linting. Given that in scope of this issue is to lint code snippets in the complete project documentation documentation , a process for iteratively approaching linting is essential.

Questions for aligning on naming

Motivation Questions for producing Code snippets

Motivation

Main motivation is to understand the process how the code snippets as is today are created inside the docs / core folder, e.g. tracer code snippets . Then understand where the code snippets for complete project documentation can be located and then understand the level of control in linting configuration design.

Questions - Proposal

Question 1

AS IS today, are the code snippets located in docs / core folder copied from code examples defined in Tsdoc comments or api documentation and pasted in markdown file located in docs / core folder and then adjusted directly as markdown code blocks to fit the needs of the complete project documentation ?

Or are the author(s) creating temporary files as code snippets out of band and then pasting code blocks in markdown ?

Which is the preferred method (AS SHOULD BE) for producing code snippets,

  • typing directly as markdown code blocks or

  • creating files as code snippets ?

Proposal 1

If the preferred method is typing directly as markdown code blocks, then

linting TypeScript and/or JS inside Markdown can be a solution approach taking under consideration

If the preferred method is creating files for the code snippets and then pasting code blocks in markdown, then

linting Typescript / JS is a straightforward approach taking under consideration

  • the Linting Recommendation described in the section Recommendations, linting design with scalability and flexibility

  • the location for persisting the files as code snippets.

Example docs / core folder:

  • docs/core/tracer/tracer.md,
  • docs/core/tracer/code-snippets/usage.ts
  • docs/core/tracer/code-snippets/usage-template.yaml

Or Example near to release packages code :

  • packages/tracer/code-snippets/usage.ts
  • packages/tracer/code-snippets/usage-template.yaml

Question 2

Some code snippets created for complete project documentation are similar however not the same in comparison to the code examples of the api documentation . What are the main differences ? As example are the types in the code snippets for complete project documentation more relaxed , e.g usage of any type in code snippets allowed however not allowed in code examples of the api documentation ?

Proposal 2

  • the Linting Recommendation described in the section Recommendations, linting design with scalability and flexibility

Question 3

Are the code snippets introduced in this PR the same as the code snippets located in docs / core folder which are rendered in the complete project documentation ? Can the code snippets be considered as ready for linting ? If not how does the process look like to make them ready for linting ?

Question 4

In case new code snippets should be created for complete project documentation how does the process look like ? E.g. a new code example with the tag @example inside TSDoc comments in released code is produced or code example in the api documentation is introduced. Then which steps follow to create code snippet for the complete project documentation ?

Recommendations, linting design with scalability and flexibility

Given powertools is a monorepo setup for managing the workspaces defined in the packages folder: tracer, logger, etc.

And powertools repository has folders that are managed independently , e.g. docs, publish layer, examples

Then

  • should 4 independent linting configurations (note: configurations !== files) exist as follows:
    • linting packages (tracer, logger, etc, .. )
    • linting examples (cdk, sam, etc)
    • linting publish layer
    • linting code snippets

What changes do I recommend for applying the 4 independent linting configurations ?

Recommendation 1:

applicable if code snippets is decided to be located near the production code

  • leverage the flat design approach of eslint by using overrides functionality of eslint.

  • introduce an overrides section explicit for code snippets inside the root elint configuration. This eslint configuration is today responsible for linting the packages (tracer, logger, etc, .. )

  • reading the overrides section for code snippets inside the linting configuration for packages is a powerfull combination.

  • A reader can know explicit which linting configuration applies for release code and which for code snippets.

  • Are the authors of release code and code snippets different entities then they can reference to a single file and have common language.

Recommendation 2:

applicable if code snippets is decided to be located near the docs/core

  • introducing a separate file for linting code snippets.
  • The new separate file can be configured to be in scope only for the code snippets and
  • file configuration can be located as near to the code snippets .

Corrections needed:

Reason is to avoid merging side effects of eslint and leverage best practices of eslint.

The .eslintignore in the root of the monorepo should define declaratively that the .eslintrc.js configuration in the root of the monorepo is about the artifacts managed by monorepo workspaces. Those are the artifacts in packages folder.

This in short means:

  • In case code snippets are located in the core/docs folder then .eslintignore ignores docs, examples and layer-publisher. . folders are ignored by default (eslint implementation)

Or

  • In case code snippets are located in the packages folder then .eslintignore ignores examples and layer-publisher. . folders are ignored by default (eslint implementation)

The linting configuration of layer-publisher and of examples should be scoped closer to the files that should be linted.

And that scope definition should be declarative , not imperative and eliminate the merging side effects of eslint .

So both of the linting configurations for layer-publisher and for examples should change to:

  • introduce the field root: true and
  • correct the package script by removing the option --resolve-plugins-relative-to .

niko-achilles avatar Dec 31 '22 02:12 niko-achilles

Hi @niko-achilles, thanks for the extensive breakdown.

For the purpose of this issue and task, let's focus exclusively on the code snippets present in the documentation. By this, I mean code snippets like the one below: image which corresponds, once rendered, to this page/section: link and that has its source here: link.

All other code samples/examples are to be excluded:

  • We are not going to lint/extract the snippets in the jsdocs annotations (code examples) that live within the code (not sure this is even possible) and that are subsequently rendered into the api docs.
  • We are already linting/checking the code under examples/* - each example has its own .eslintrc.js file and it's running linting on specific points of the CI/CD using the --resolve-plugins-relative-to . We can discuss changes to this in a separate issue and in the future.

All other parts of the documentation (i.e. Markdown) are also to be excluded, we are only going to lint the code snippets that are written in TypeScript. This means we are also going to exclude all YAML (SAM templates) or other types of snippets.


When talking about code snippets inside the documentation (see image above for reference), today we just write the code inside the markdown files. This is probably part of the reason why sometimes they contain errors.

I'm in favor of extracting these code snippets in their own files if that helps linting them.

Powertools for Python is already doing this, so clearly the documentation engine we are using supports this. Ideally I would like to have a folder under docs (i.e. docs/snippets) where all these snippets live. Then, in the markdown, we could import them like this: image

So to recap:

  • We only want to focus on the code snippets written in TypeScript and inside the documentation
  • We can extract each snippet in its own file
  • It's acceptable to make minimal changes to the snippets so that they can be linted (if linting is failing)
  • We are going to tackle one documentation page in each PR (i.e. the Tracer page should be 1 PR, the Logger should be another, etc.)
  • Once all code snippets are extracted, their PR merged, and we have validated that the documentation works/looks the same. We'll do another PR to start applying the linting to these files. This PR will use the existing eslint rules (regardless of whether they are OK or not).
  • Once the point above is merged & integrated with the CI/CD process, we will open a new issue to discuss any change to the eslint rules that might affect or not the whole project

So, essentially, we'll have (for now) 4 PRs:

  • docs(tracer): extract code snippets in separate files
  • docs(logger): extract code snippets in separate files
  • docs(metrics): extract code snippets in separate files
  • chore(docs): apply current eslint rules to files under docs/snippets

The goal of separating the work like this, and maintaining very gradual is to:

  • ensure that the adoption of this new mechanism is gradual & incremental
  • ensure that we don't take away focus from other efforts going on in the project
  • ensure that we don't make too many changes that will risk breaking everything and/or causing huge diff in the PRs

If you agree with this approach and everything is clear, please let me know and I'll open 4 new issues (one for each PR) as described above. Once they are created, you can start working whenever you want.

dreamorosi avatar Jan 09 '23 16:01 dreamorosi

So to recap: We only want to focus on the code snippets written in TypeScript and inside the documentation We can extract each snippet in its own file We are going to tackle one documentation page in each PR (i.e. the Tracer page should be 1 PR, the Logger should be another, etc.)

And

So, essentially, we'll have ... PRs: docs(tracer): extract code snippets in separate files docs(logger): extract code snippets in separate files docs(metrics): extract code snippets in separate files

@dreamorosi means

  • we buy the approach to create typescript code snippets in their own files so that we can lint them.
  • we drop the idea to lint typescript code snippets as markdown code blocks with the tool: eslint plugin markdown.

I agree to open new issues for

  • docs(tracer): extract code snippets in separate files
  • docs(logger): extract code snippets in separate files
  • docs(metrics): extract code snippets in separate files

however to the issues the locations of the code snippets as separate files should be made explicit.

Now for the location of the code snippets and where the eslint configuration for code snippets live i have some questions to make see below

So, essentially, we'll have ... PRs: chore(docs): apply current eslint rules to files under docs/snippets

@dreamorosi please note that the docs/snippets does not belong to the workspaces managed by monorepo . It would be an anti-pattern to use node_nodules and the eslint-config of the workspaces as file located here.

Questions :

  1. Is it ok to install node_modules in docs/snippets ? Note the node_modules installed for the monorepo are for the workspaces and NOT for other folders like docs, etc even if i as dev. can reference them .

  2. And also is it ok to create an eslint-config with the exact same rules as applied in the file, however managing the scope of this separate eslint-config file in docs/snippets by introducing the root : true field ?

@dreamorosi if Questions 1,2 is an ok then i agree with creating an issue as chore(docs): apply current eslint rules to files under docs/snippets

and also i agree with creating issues, concatenating the location of the files in the title (something like the following):

  • docs(tracer): extract code snippets in separate files, docs/snippets
  • docs(logger): extract code snippets in separate files, docs/snippets
  • docs(metrics): extract code snippets in separate files, docs/snippets

@dreamorosi if Questions 1,2 are NOT ok then

  • we buy the option to use node_modules of the workspaces also for the docs folder ( without installing node_modules in the docs/snippets folder).
  • we buy the option to use same eslint-config as file located here also for the docs/snippets .

Please note Questions 1,2 are important because we need an other issue for improving the eslint configuration attributes which are NOT rules.

See here background , lessons learned from the author of eslint himself: link to blog article

The attributes like root: true, location of eslint files , package.json commands ( e.g: --resolve-plugins-relative-to ?! ) and eslintignore glob patterns are attributes that can improve the management of eslint configuration for workspaces in the monorepo and for folders outside the workspaces of the monorepo, e.g docs/snippets folder.

For the other points:

Once all code snippets are extracted, their PR merged, and we have validated that the documentation works/looks the same. We'll do another PR to start applying the linting to these files. This PR will use the existing eslint rules (regardless of whether they are OK or not).

@dreamorosi for the rules i agree . I can use the same rules for code snippets that are used for the code under packages.

Once the point above is merged & integrated with the CI/CD process, we will open a new issue to discuss any change to the eslint rules that might affect or not the whole project

@dreamorosi ok i understand for the eslint rules.

niko-achilles avatar Jan 09 '23 17:01 niko-achilles

  • we buy the approach to create typescript code snippets in their own files so that we can lint them.
  • we drop the idea to lint typescript code snippets as markdown code blocks with the tool: eslint plugin markdown.

Correct.

please note that the docs/snippets does not belong to the workspaces managed by monorepo . It would be an anti-pattern to use node_nodules and the eslint-config of the workspaces as file located here.

Okay, I see your point. You are right.

Questions :

  1. Is it ok to install node_modules in docs/snippets ? Note the node_modules installed for the monorepo are for the workspaces and NOT for other folders like docs, etc even if i as dev. can reference them .

It's okay, but please see my question below (*).

  1. And also is it ok to create an eslint-config with the exact same rules as applied in the file, however managing the scope of this separate eslint-config file in docs/snippets by introducing the root : true field ?

Yes.


Question: should we make the new docs/snippets folder part of the npm workspace? If we don't, the documentation will always behind the released packages on npm.

For example:

  • Step 1: we are at v1.0 on NPM
  • Step 2: we add a new feature to one of the utilities (this change is not released / published on npm yet)
tracer.newFeature();
  • Step 3: we add a new code snippet to the docs that uses the unreleased feature
  • Step 4: linting will fail because the tracer.newFeature() method doesn't exist in that installed version
  • Step 5: we need to make a release, publish v2 on NPM
  • Step 6: we can now update the docs (that we tried at step 3) using the published version

What do you think? Am I missing something?

dreamorosi avatar Jan 10 '23 09:01 dreamorosi

Question: should we make the new docs/snippets folder part of the npm workspace?

@dreamorosi yes , given in the Steps 1,2,3,4,5,6 described above in your comment, i understand that the lifetime of a code snippets follows the lifetime of the published code of workspace .

And also i can reference in package.json the node_modules already installed for the workspace And also use the powertools version that are released .

And also i can use the same eslint config file , located here that is used for the workspace .

Please create the issues and mention explicit where the location of the code snippets should be for the following 4 PRs identified, so that i can start work on them:

  • docs(tracer): extract code snippets in separate files. e.g. packages/docs/snippets/tracer/*.ts
  • docs(logger): extract code snippets in separate files, e.g. packages/docs/snippets/logger/*.ts
  • docs(metrics): extract code snippets in separate files, e.g. packages/docs/snippets/metrics/*.ts
  • chore(docs): apply current eslint rules to files under docs/snippets

niko-achilles avatar Jan 10 '23 11:01 niko-achilles

I have created the first batch of issues: #1219 #1220 #1221

Whenever you are ready you can leave a comment under each one (one at the time), so that I can assign it to you. After that you can start working on it.

I'll create the fourth later on, when we are almost done with these three.

dreamorosi avatar Jan 10 '23 17:01 dreamorosi

I'll create the fourth later on, when we are almost done with these three.

@dreamorosi we will need an other one in context of linting and when a developer as contributor uses gitpod This extension could be removed from https://github.com/awslabs/aws-lambda-powertools-typescript/blob/3751f318a401d85473beec085b005203f5dc6c7c/.gitpod.yml#L21

niko-achilles avatar Jan 11 '23 23:01 niko-achilles