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

fix: use common conventions: tsconfig.json, package.json #2602

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

rwat17
Copy link
Contributor

@rwat17 rwat17 commented Aug 11, 2023

  • change package.json and tsconfig.json files to follow common
    convention

Peter's additions to fix the broken build:

  • Included shelljs typings as a dev dependency for the package called
    cactus-plugin-ledger-connector-go-ethereum-socketio
  • Also included "socket.io-client" as another missing dev dependency for
    the package above.

Closes: https://github.com/hyperledger/cacti/issues/2216

Co-authored-by: Peter Somogyvari peter.somogyvari@accenture.com

Signed-off-by: Tomasz Awramski tomasz.awramski@fujitsu.com
Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com

@rwat17
Copy link
Contributor Author

rwat17 commented Aug 11, 2023

Also sorted them with $npm run custom-checks from other PR :) https://github.com/hyperledger/cacti/pull/2434

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@rwat17 Mostly LGTM, please see my comments above!

Also sorted them with $npm run custom-checks from other PR :) #2434

For future reference
(it's OK for this one because I'm trying to be a little less annoying with my nit-picking)
When you combine two separate changes when one is relatively small and the other one is relatively big, it makes it hard to interpret the changes later. E.g., next year if you come back tyring to figure out something specific about the way you changed the compiler root directory, you'll have to dig through the hundreds of lines of diff from the sorting which could've been in a separate commit not shadowing the meaningful changes that have actual impact on the build system (the sorting is important for develoer experience too but it does not actually change anything so it's just noise when someone is looking at the diff)

Copy link
Contributor

@sandeepnRES sandeepnRES left a comment

Choose a reason for hiding this comment

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

LGTM, but please address peter's comment.
Also all the package.json files in examples that are in the diff are just re-arranging of lines, and nothing new in that, would recommend to revert them, but not strongly against it.

@rwat17 rwat17 force-pushed the common-package-convention branch from f15c8d2 to d46e5ea Compare August 16, 2023 14:47
@rwat17
Copy link
Contributor Author

rwat17 commented Aug 16, 2023

@petermetz I agree about not sorting packages in this commit, it is not very relevant and not purpose of this one. It should be separated. I think it will be better to sort it after I fix my other PR about not working sorting packages functionality. Please review this changes and tell me if I have to revert what Sandeepn mentioned too :)

@rwat17 rwat17 requested a review from petermetz August 16, 2023 15:05
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@rwat17 Awesome, thank you very much for going the extra mile to decouple the sorting changes from the rest of it! LGTM

@petermetz petermetz enabled auto-merge (rebase) August 17, 2023 06:16
@petermetz petermetz force-pushed the common-package-convention branch 2 times, most recently from 7590e80 to 33bf0f0 Compare August 17, 2023 17:09
- change package.json and tsconfig.json files to follow common
convention

Peter's additions to fix the broken build:
- Included shelljs typings as a dev dependency for the package called
cactus-plugin-ledger-connector-go-ethereum-socketio
- Also included "socket.io-client" as another missing dev dependency for
the package above.

Closes: hyperledger-cacti#2216

Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Signed-off-by: Tomasz Awramski <tomasz.awramski@fujitsu.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz petermetz force-pushed the common-package-convention branch from 33bf0f0 to 334a9e6 Compare August 18, 2023 02:45
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@rwat17 FYI: I fixed the build just now - there were missing dependencies. I also made an attempt at making the commit subject a little more specific

@petermetz petermetz changed the title fix: use common package convention fix: use common conventions: tsconfig.json, package.json Aug 18, 2023
@petermetz petermetz merged commit 50f5c02 into hyperledger-cacti:main Aug 18, 2023
@rwat17
Copy link
Contributor Author

rwat17 commented Aug 18, 2023

@petermetz Thanks and sorry for my oversights

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.

fix(connector-socketio): use common package convention
3 participants