Azure / azure-iot-sdk-node

A Node.js SDK for connecting devices to Microsoft Azure IoT services
https://docs.microsoft.com/en-us/azure/iot-hub/
Other
262 stars 227 forks source link

Users/danhellem/device samples #1085

Closed danhellem closed 2 years ago

danhellem commented 2 years ago

DO NOT PUBLISH THIS, PR ONLY FOR FEEDBACK

Checklist

Reference/Link to the issue solved with this PR (if any)

These are for our new device samples format and content

danhellem commented 2 years ago

This looks great! Some nits:

  • could the spaces be removed from the directory names (e.g. getting started -> getting_started)

Done

  • In directories with multiple examples (e.g., send message, device twins), I think it'd better to put JS and TS examples in their own subdirectories

Lets talk about this. I like having the TS files side by side with JS files.

  • Do we want to have users pull the package from NPM or link the local source code with Lerna? Right now there is no Lerna linking, and dev-environment.md just has the user pull from NPM. Maybe we want to keep it that way, but it's something to consider.

I think the user should have to pull from NPM. Just like the would in the real world.

  • Minor spelling/grammar issue: In a few places, "setup" should be "set up" and "environmental variable" should be "environment variable"

Fixed, i think :)

BertKleewein commented 2 years ago

I love this! We really need this kind of organization and I'm glad that you're taking this on.

Lets talk about this. I like having the TS files side by side with JS files.

I was about to make a similar comment. I tend to agree with Vishnu, especially since we have hand-written JS, and we have compiler-generated JS, and the compiler-generated JS is more closely related to the TS, but it's further away in the file system.

Plus the src directory naming convention is more closely aligned with TS. though I've also seen it used with JS, especially when using webpack-type tools

Also, I don't know the answer, but do we want device_samples/getting_started/src/connections or do want device_samples/getting_started/connections/src? The latter makes more sense to me, but you've though about it more than I have.


In reply to: 1005128467

vishnureddy17 commented 2 years ago

Also, I don't know the answer, but do we want device_samples/getting_started/src/connections or do want device_samples/getting_started/connections/src? The latter makes more sense to me, but you've though about it more than I have.

In reply to: 1005128467

+1

YoDaMa commented 2 years ago

reviewing this... I agree with the comments already made. I'll see what else can be said. Once this is merged into a branch on the main repo I can also contribute some changes.

danhellem commented 2 years ago

Also, I don't know the answer, but do we want device_samples/getting_started/src/connections or do want device_samples/getting_started/connections/src? The latter makes more sense to me, but you've though about it more than I have. In reply to: 1005128467

+1

I fixed this in the branch I am going to push in a bit