Docs: improve the "Contributing" section documentation about aws-lambda-powertools-typescript HOT 30 CLOSED

Adding GitPod or CodeSpaces support is really handy .

eg: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md

from aws-lambda-powertools-typescript.

Did a quick test with devcontainer in VSCode, leaving this here for future reference:

.devcontainer/Dockerfile

ARG VARIANT="16"
FROM mcr.microsoft.com/vscode/devcontainers/javascript-node:0-${VARIANT}

RUN mkdir ~/.npm-global && \
  npm config set prefix '~/.npm-global' && \
  echo "export PATH=~/.npm-global/bin:$PATH" >> ~/.profile

ARG EXTRA_NODE_VERSION=16.5.0
RUN su node -c "source /usr/local/share/nvm/nvm.sh && nvm install ${EXTRA_NODE_VERSION}"

.devcontainer/devcontainer.json

{
  "name": "Node.js",
  "build": {
    "dockerfile": "Dockerfile"
  },
  "extensions": [
    "dbaeumer.vscode-eslint",
    "esbenp.prettier-vscode",
    "visualstudioexptteam.vscodeintellicode",
    "amazonwebservices.aws-toolkit-vscode",
    "github.copilot",
    "firsttris.vscode-jest-runner"
  ],
  "postCreateCommand": "npm install && npm run lerna-bootstrap && npm run lerna-test"
}

from aws-lambda-powertools-typescript.

Agree that it needs improvement. The AWS Solutions Constructs repo, in my opinion, has a good example of this file.

Also interesting is their DESIGN_GUIDELINES.md file which we could also adopt a form of.

The process they request contributors to follow requires an issue and a design document to be reviewed by a maintainer before someone starts to implement a change. For trivial changes just the issue could suffice but for bigger changes I think it's a good idea. On one hand the maintainer review could become a bottleneck but in the spirit of a good experience as a contributor it'd be far worse to invest time and effort in an implementation that'd be rejected when it becomes a PR.

from aws-lambda-powertools-typescript.

I like @dreamorosi's suggestion of the AWS Solutions Constructs example, and think we could get a good first document by using and adapting the example for our repository. I'll be happy to start a draft.

I don't have much experience around design guidance, and am quite new to this project. So for now I'll be passive regarding design guidelines.

from aws-lambda-powertools-typescript.

Another important point to document (and possibly also setup in our project configuration) is the versions of:

• NPM
• Node.js
  that you need to have installed locally to have the project properly working.
  I am in favour of documentation + local setup scripts/hooks to avoid errors related to wrong versions being used.

from aws-lambda-powertools-typescript.

I would not mind adding a PR for gitpod / CodeSpaces support. To help onboard contributors.

from aws-lambda-powertools-typescript.

We should also include build steps they can run themselves to ensure proper linting (and maybe add a step in the PR build to verify if it's alright).

Agree on enforcing it.
Using camel casing is something we could also document here #115 because I can see how someone new to the project could do a similar thing.

#102 (comment)

I think we're reaching a point where we need to recollect all the ideas, and trim them down into individual tasks.

from aws-lambda-powertools-typescript.

Another item we could document: testing style.
#102 (comment)

from aws-lambda-powertools-typescript.

I think the approach here seems sensible, and yes more documentation on how things should work the better!

from aws-lambda-powertools-typescript.

Additional contributing section: more elaborated PR-related guidance and best practices, including but not limited to:

• meaningful PR titles
• PR titles following our conventional commit semantics
• Do not open PRs if you haven't created an issue first and that issue has been discussed with and approved by at least one of the maintainers

from aws-lambda-powertools-typescript.

I do like how python powertools has a simple Makefile

ie:

• make dev - sets up and installs all of the dev dependencies and pre-commit hooks
• make pr - does all of the tasks recommended before submitting a PR and includes
  • make test - all unit tests including performance tests
  • make lint - does all of the linting and security checks
  • make format - does all of the code formatting
• make docs-local - for the docs and does not need any docker stuff :P

from aws-lambda-powertools-typescript.

Another aspect that we should add in our contributing section: avoiding creating PR's that include code changes that are not related to the specific issue they are trying to address.
Housekeeping shows good intentions, but it does not help us maintainers keeping track of the changes that are released and it's better to create a separated issue/PR for changes unrelated to the original scope.

Also we should clarify what's the best way to define a conversation resolved in a PR review :)

from aws-lambda-powertools-typescript.

Should we put contributing section in the doc? It's easier to locate and search. But the drawback is that the content has to be in the docs folder.

I personally would prefer to be just a link to CONTRIBUTING.md from doc. Dev will first look at this file at root folder of repo.

from aws-lambda-powertools-typescript.

I think a good starting point for the design principles could be the tenets described in the Python's version of the project. Other than that I think good principles could be feature parity and consistency with other runtimes.

from aws-lambda-powertools-typescript.

@bahrmichael @dreamorosi thanks for your input, I completely agree.

I would also add: in our contributing guidelines we should also clarify how to find Issues, PR's (through the Github projects). Additionally we could define what "ownership" (= assignees) of an issue is and what that means in practice.

Here's my proposal:

1. When someone creates a new Issue, there is no owner assigned because the issue needs to be triaged still (we might or might not implement it for example). To get feedback of the maintainers, we can tag them in the issue's comments to get their input (and/or ping them on slack of course). So the issue has no owner (assignee) yet.
2. Once the issue is triaged and ready to be picked up, 1 or more owners can be assigned in the Assignees section. An owner of an issue has the responsibility of carry it to completion and bring it to the done column.

I myself am guilty of assigning issues to get people's attention so not following what suggested above, by the way. But I realised that this was causing me confusion.

I think documenting this etiquette will bring more clarity about who is working on what in the future. But first we need of course to agree whether this is the best way of working/approach or there are better ways.
What do you think?
cc: @alan-churley

from aws-lambda-powertools-typescript.

100% agree with this, and it's great to document it so that we all know what's the proper etiquette. To double down on this I think we could codify also how PRs should work. For those I think the current behaviour is that the assignee is the person who opened the PR and is responsible of resolve any comment/request from the reviewers. Reviewers are those who are expected to review the PR and provide feedback/approve/reject the changes.

What do you think?

from aws-lambda-powertools-typescript.

As suggested by @alan-churley this issue can include the setup of pre-commit hooks that would validate/flag errors

from aws-lambda-powertools-typescript.

There's been a lot of great feedback, so I'll try to summarize it into some areas that we can work through bit by bit.

• Project Configuration: #115 (comment)
• Design Guidelines
  • https://github.com/awslabs/aws-solutions-constructs/blob/main/DESIGN_GUIDELINES.md
  • Tenets from https://awslabs.github.io/aws-lambda-powertools-python/latest/
  • "Other than that I think good principles could be feature parity and consistency with other runtimes."
• "clarify how to find Issues, PR's (through the Github projects)." #115 (comment)
• "define what "ownership" (= assignees) of an issue is and what that means in practice" ttps://github.com//issues/115#issuecomment-884737311
• How to do PRs: #115 (comment)
• Code Style
  • Linting: #115 (comment)
  • Testing: #115 (comment)
  • Pre-commit hooks: #115 (comment)

from aws-lambda-powertools-typescript.

⚠️ COMMENT VISIBILITY WARNING ⚠️

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.

from aws-lambda-powertools-typescript.

New item: code readability.
See: https://github.com/awslabs/aws-lambda-powertools-typescript/pull/102/files/984da5311a4a396d1b56538e2b57204dd9ffb0dc#r679917689

from aws-lambda-powertools-typescript.

Other example of nice/exhaustive contributing section in this repo, also this uses GitHub Wiki instead of a CONTRIBUTING.md file.

from aws-lambda-powertools-typescript.

Additional section - etiquette related to issues assigned to a member of the community.

Inspired by this thread: https://twitter.com/aritdeveloper/status/1478098181351223302?t=Qw8BKdxsvIpGGn1VnF3eOQ&s=19

from aws-lambda-powertools-typescript.

Should we put contributing section in the doc?
It's easier to locate and search. But the drawback is that the content has to be in the docs folder.

from aws-lambda-powertools-typescript.

normally it is in the root dir

from aws-lambda-powertools-typescript.

I did one for the python project too, makes it super easy to contribute

CodeSpaces config:

• https://github.com/aws/aws-cdk/blob/master/.devcontainer.json

GitPod config:

• https://github.com/aws/aws-cdk/blob/master/.gitpod.yml

from aws-lambda-powertools-typescript.

Just FYI, I created a separate ticket for the scope of "local development" (new contributors onboarding)

#426

from aws-lambda-powertools-typescript.

There is still typos, python references and aws cdk project references.

from aws-lambda-powertools-typescript.

Example of referencing tests/e2e which does not exist in the repo (maybe this references within the packages/logger/tests/unit ?)

Python references, for which there still needs to be a Typescript version of this.

from aws-lambda-powertools-typescript.

Most of the items are addressed (Python references are also removed). With @AWSDB PR, we're confident for Production Release with this doc.

Any other future improvement on CONTRIBUTING.md will be created as a seperated issue.

from aws-lambda-powertools-typescript.

⚠️ COMMENT VISIBILITY WARNING ⚠️

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.

from aws-lambda-powertools-typescript.