examples icon indicating copy to clipboard operation
examples copied to clipboard

express examples

Open gireeshpunathil opened this issue 5 years ago • 10 comments

Fixes: https://github.com/nodejs/examples/issues/10

/cc @bnb

gireeshpunathil avatar Jul 17 '20 13:07 gireeshpunathil

Will take a more in-depth look soon here (probably Monday) but one tiny piece of feedback is that you're missing README.md for the added directories (see the countEntriesInDirectory CLI example and its parent directory, yargs as an reference for adding both an example and a category)and a GItHub Actions CI file (see .github/workflows/cli-yargs-countentriesindirectory.yml as a reference) for this specific project.

I do need to update the CONTRIBUTING.md to address the CI and testing tooling steps as it currently stands, my bad.

bnb avatar Jul 17 '20 16:07 bnb

@bnb - added a readme, aligning to the scope and purpose of this example. PTAL!

gireeshpunathil avatar Jul 18 '20 06:07 gireeshpunathil

Retargeted to main, will check this as soon as I can - have been slammed with some other priorities recently ❤️

bnb avatar Aug 07 '20 19:08 bnb

Additionally, we'd want to add a CI file for this project before merging 👍🏻

bnb avatar Sep 25 '20 19:09 bnb

@bnb - thanks! I guess I have addressed most of your comments. In addition,

ideally we'd have this in a directory within the servers/express/ directory - so something like servers/express/basics or servers/express/buffet that gives an example of what's contained within. also change the name property in package.json to match this

moved the content into basics folder, and renamed the app ih the package.json, pls have a look and let me know if that is what you meant.

the license should probably match the repo, which is MIT

done, removed all license content. I guess the one in the repo vortex folder would suffice.

testing locally, the file upload did not work

The /upload assumes there is an uploads folder, and if it is missing, it throws. There are two ways to solve this:

  • document this
  • pre-create a folder

wdyt?

Additionally, we'd want to add a CI file for this project before merging

any pointers? I have never made one.

thanks once again!

gireeshpunathil avatar Sep 29 '20 06:09 gireeshpunathil

The /upload assumes there is an uploads folder, and if it is missing, it throws. There are two ways to solve this:

  • document this
  • pre-create a folder

wdyt?

I'd say both if we can :P

bnb avatar Oct 12 '20 18:10 bnb

On pointers for CI, I'd say start with the current CI workflow that we have - you basically just need to adapt it:

https://github.com/nodejs/examples/blob/main/.github/workflows/cli-yargs-countentriesindirectory.yaml

There's also the official reference and docs:

https://docs.github.com/en/free-pro-team@latest/actions/reference https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions

I've also written a blog post on GitHub Actions CI that's relatively comprehensive and may be able to help answer any questions:

https://dev.to/bnb/an-unintentionally-comprehensive-introduction-to-github-actions-ci-blm

bnb avatar Oct 12 '20 18:10 bnb

@bnb - I have pushed changes pertinent to out discussion, please take a look!

gireeshpunathil avatar Nov 13 '20 14:11 gireeshpunathil

Seems that tap isn't getting installed which is failing the build. If you could figure out why, that'd be awesome :)

bnb avatar Dec 03 '20 19:12 bnb

Looks like one of the tests is failing 😅

bnb avatar Dec 10 '20 17:12 bnb