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

doc: update information about target property #499

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Conversation

groozin
Copy link
Contributor

@groozin groozin commented Oct 10, 2023

Fixes #491

Checklist

@@ -2,3 +2,4 @@
node_modules/
.idea
.nyc_output
.tap/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Fdawgs

does tap generates artifacts?

Copy link
Contributor Author

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")

image

Copy link
Contributor

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

Copy link
Member

@Fdawgs Fdawgs Oct 10, 2023

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to remove the .tap/ from the ignore and just not adding the artifacts to the PR. This PR is really just about the doc/readme. And I don't want to introduce additional confusion here.
I guess the topic of what to put in gitignore can be solved elsewhere. WDYT @Uzlopak @Fdawgs ?

Copy link
Member

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.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 10, 2023

Please remove unnecessary changes

Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

LGTM

@simoneb
Copy link
Collaborator

simoneb commented Oct 10, 2023

I'm happy with the changes but @Uzlopak @Fdawgs please shout if you'd like to remove the .tap exclusion in .gitignore. I don't see any harm in leaving it there though

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 10, 2023

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.

@groozin
Copy link
Contributor Author

groozin commented Oct 10, 2023

the dist comes from the husky hook - I assumed that was by design. I can try and maybe do it in another way.

@simoneb
Copy link
Collaborator

simoneb commented Oct 10, 2023

good point @Uzlopak , @groozin is working with me so I don't have any concerns in that regard, but yes @groozin revert the change in the dist folder please, if there is a change needed it will be done by the publishing process

@groozin groozin requested review from Uzlopak, Fdawgs and simoneb October 10, 2023 20:29
@simoneb
Copy link
Collaborator

simoneb commented Oct 10, 2023

@Uzlopak @Fdawgs if you're happy with the change, merge away

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Uzlopak Uzlopak merged commit 4278b29 into fastify:main Oct 10, 2023
3 checks passed
@github-actions github-actions bot mentioned this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify meaning of "target" option
4 participants