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

[jsscripting] Add info about npm requiring path #17022

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Conversation

SkyLined
Copy link
Contributor

@SkyLined SkyLined commented Jul 8, 2024

The command to install an npm package that was suggested in the documentation would not work, as it was missing the required path. I've added information to explain this is required.

The command to install an npm package that was suggested in the documentation would not work, as it was missing the required path. I've added information to explain this is required.

Signed-off-by: SkyLined <[email protected]>
@SkyLined SkyLined requested a review from florian-h05 as a code owner July 8, 2024 08:18
@jlaur jlaur changed the title Add info about npm requiring path [jsscripting] Add info about npm requiring path Jul 8, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Jul 8, 2024

As step 4 is already asking to do the command ‘ from your library's folder’, adding the path at step 5 is not needed. Or what am I missing?

@florian-h05
Copy link
Contributor

I think the point here is, that the tarball is in the library folder and not inside automation/js.
But instead of adding this to the command, I would rather add „copy your tarball to automation/js“ or „make sure to provide the path to your tarball to install it“. Though I would expect that this is self explaining …

@SkyLined
Copy link
Contributor Author

Having a copy of the tarball in automation/js makes no sense to me; you can just leave it where it is and provide the path.

Assuming something is self-explanatory is the most common flaw in documentation, please do not leave out information unless it really clutters the page (in which case, link to another page that provide it). In this case, it doesn't clutter but is a useful reminder to provide the path.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

LGTM

@lsiepel lsiepel merged commit 643b4e8 into openhab:main Jul 10, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Jul 10, 2024
@florian-h05
Copy link
Contributor

@SkyLined Can you please open a PR to openhab-js to apply the change there as well?

psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Jul 12, 2024
The command to install an npm package that was suggested in the documentation would not work, as it was missing the required path. I've added information to explain this is required.

Signed-off-by: SkyLined <[email protected]>
florian-h05 added a commit to florian-h05/openhab-js that referenced this pull request Jul 13, 2024
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
The command to install an npm package that was suggested in the documentation would not work, as it was missing the required path. I've added information to explain this is required.

Signed-off-by: SkyLined <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
The command to install an npm package that was suggested in the documentation would not work, as it was missing the required path. I've added information to explain this is required.

Signed-off-by: SkyLined <[email protected]>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
The command to install an npm package that was suggested in the documentation would not work, as it was missing the required path. I've added information to explain this is required.

Signed-off-by: SkyLined <[email protected]>
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
The command to install an npm package that was suggested in the documentation would not work, as it was missing the required path. I've added information to explain this is required.

Signed-off-by: SkyLined <[email protected]>
Signed-off-by: Ciprian Pascu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants