Closed michaelbrewer closed 1 year ago
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:
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.
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:
- If we separate the files can we still see them in context when authoring the docs locally?
- Would each code snippet have its own file?
- Would it be possible to use the same exact linting rules that we use for the rest of the project?
- 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 fordocs/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.
@dreamorosi example PR is up #730
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.
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..
@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:
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.
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.
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.
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:
template.yml
example, by adding missing fields (handler
, AWSTemplateFormatVersion
etc.)Context
tracing https requests exampleHi @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 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.
Sure, the aws-sdk
is already a dev dependency so it won't be an issue. Thanks for your time!
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:
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 i can pick up this issue .
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 reading at the comments , are the following points the scope of this issue ?
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 .
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 , 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:
So the next step would be to bring the approaches for discussion.
Minor Recommendations:
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 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 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:
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"
],
...
}
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)
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 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)
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.
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
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.
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.
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 ?
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
the Linting Recommendation described in the section Recommendations, linting design with scalability and flexibility
type aware linting is not supported, See eslint plugin markdown.
Note: the code base of powertools does not use type aware linting rules.
linting SAM cloudformation yaml inside markdown is not applicable out of the box. However applicable with custom tooling, e.g. extracting yaml code blocks defined in markdown and then linting with cfn-lint and serverless rules.
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:
Or Example near to release packages code :
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 ?
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 ?
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 ?
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
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
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:
core/docs
folder then .eslintignore ignores docs, examples and layer-publisher. . folders
are ignored by default (eslint implementation)Or
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:
root: true
and--resolve-plugins-relative-to .
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:
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:
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:
So to recap:
So, essentially, we'll have (for now) 4 PRs:
docs/snippets
The goal of separating the work like this, and maintaining very gradual is to:
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.
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
I agree to open new issues for
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 :
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 .
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):
@dreamorosi if Questions 1,2 are NOT ok then
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.
- 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 :
- Is it
ok
to install node_modules indocs/snippets
? Note the node_modules installed for the monorepo are for the workspaces andNOT
for other folders like docs, etc even if i as dev. can reference them .
It's okay, but please see my question below (*).
- 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 indocs/snippets
by introducing theroot : 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:
v1.0
on NPMtracer.newFeature();
tracer.newFeature()
method doesn't exist in that installed versionv2
on NPMWhat do you think? Am I missing something?
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:
I have created the first batch of issues:
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.
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
I have created two more issues:
The first one has already been taken care of and its corresponding PR merged (#1251). The second one is up for grabs once #1245 has also been merged.
Once #1252's PR is merged, we can finally close this issue as complete.
@dreamorosi new commit for #1245. When merged i can start with #1252 .
It appears we had a regression when working on the Logger docs: #1253
Any chance you could fix it?
@dreamorosi because we track in this issue the relative issues to code snippets there are some typescript compiler recommendations to improve the quality of code snippets. I have identified the following:
1.
where: docs/snippets/logger/bringYourOwnFormatterHandler.ts
import { MyCompanyLogFormatter } from './utils/formatters/MyCompanyLogFormatter';
can be solved by:
// import { MyCompanyLogFormatter } from './utils/formatters/MyCompanyLogFormatter';
where: docs/snippets/logger/clearStateDecorator.ts
what:
public async handler(_event: any, _context: any): Promise<void> {
// Persistent attributes added inside the handler will NOT be cached
// across invocations
if (event['special_key'] === '123456'){
...
}
...
}
can be solved by:
public async handler(_event: any, _context: any): Promise<void> {
// Persistent attributes added inside the handler will NOT be cached
// across invocations
if (_event['special_key'] === '123456'){
...
}
...
}
}
3.
where: docs/snippets/tracer/accessRootTraceId.ts
what:
export const handler = async (event: unknown, context: Context): Promise<void> => {
try {
...
} catch (err) {
...
// Example of returning an error response
return {
statusCode: 500,
body: `Internal Error - Please contact support and quote the following id: ${rootTraceId}`,
headers: { '_X_AMZN_TRACE_ID': rootTraceId },
};
}
};
can be solved by:
export const handler = async (event: unknown, context: Context): Promise<INTRODUCE-SPECIFIC-TYPE>=> => {
try {
...
} catch (err) {
...
// Example of returning an error response
return {
statusCode: 500,
body: `Internal Error - Please contact support and quote the following id: ${rootTraceId}`,
headers: { '_X_AMZN_TRACE_ID': rootTraceId },
};
}
};
4. where: docs/snippets/tracer/disableCaptureResponseMethod.ts what:
class Lambda implements LambdaInterface {
...
}
can be solved by:
import { LambdaInterface } from '@aws-lambda-powertools/commons';
5.
where: docs/snippets/tracer/putMetadata.ts
what:
export const handler = async (_event: any, _context: any): Promise<void> => {
const res; /* ... */
tracer.putMetadata('paymentResponse', res);
};
can be solved by:
export const handler = async (_event: any, _context: any): Promise<void> => {
let res; /* ... */
tracer.putMetadata('paymentResponse', res);
};
With #1259 merged all the code snippets present in the documentation are now linted, this should help decrease the amount of typos/errors present in the docs.
We can close this issue as complete.
Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.
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
Missing or unclear documentation
Improvement
docs/
folder, to allow for mkdocs to automatically pick it upRelated existing documentation
Related issues, RFCs
Example of documentation examples with errors in them: