-
Notifications
You must be signed in to change notification settings - Fork 35
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
doc: update information about target
property
#499
Conversation
@@ -2,3 +2,4 @@ | |||
node_modules/ | |||
.idea | |||
.nyc_output | |||
.tap/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we keep the tests artifacts from tap
out of repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does tap generates artifacts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what I am getting after running npm test
> git status
On branch main
Your branch is behind 'origin/main' by 2 commits, and can be fast-forwarded.
(use "git pull" to update your local branch)
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: README.md
Untracked files:
(use "git add <file>..." to include in what will be committed)
.tap/
no changes added to commit (use "git add" and/or "git commit -a")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats strange, because we have a general .gitignore file and if tap generates artifacts we should also cover them in that file. Thats why I am pinging @Fdawgs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Uzlopak The general .gitignore doesn't have this as I don't believe any other Fastify repo generates tap artefacts.
This repo isn't kept in line with the other ones though as it's a GitHub Action and I don't know what it does/doesn't need to ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to just add your doc changes, thanks @groozin.
Please remove unnecessary changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I dont see any harm either. Just being strange that they are generated. Didnt see them till today. Only worrying me is the build dist file. Nothing against @groozin but we have to be careful and ensure that there is not a sec. vuln. Introduced by bike shedding. |
the dist comes from the husky hook - I assumed that was by design. I can try and maybe do it in another way. |
add to .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #491
Checklist
npm run test
andnpm run benchmark
and the Code of conduct