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

refactor: updated action example on readme #51

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Conversation

0xDiscotech
Copy link
Contributor

Updated README based on changes done on this PR: #50

CLOSES BES-85

@0xDiscotech 0xDiscotech requested a review from gas1cent October 24, 2023 00:21
@0xDiscotech 0xDiscotech self-assigned this Oct 24, 2023
@linear
Copy link

linear bot commented Oct 24, 2023

BES-85 Proofread the README

Make sure there is no typos in the readme and the grammar is correct.s

Then add a usage example showing how to configure export with a matrix of arguments, this can be seen in the testing repo.

@@ -4,18 +4,17 @@

# Interface Exporter Action

Interface Exporter Action automates the process of extracting TypeScript interfaces from Solidity contracts and provides compatibility with TypeChain. Developers can seamlessly generate typings with only a few lines of yaml code.
Interface Exporter Action automates the process of extracting TypeScript interfaces from Solidity contracts and interfaces and provides compatibility with TypeChain. Developers can seamlessly generate typings with only a few lines of yaml code.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we decide to call the action Solidity Exporter Action this paragraph needs to be changed

README.md Outdated
Comment on lines 13 to 17
| package_name | Chosen name for the exported NPM package | **Required** | |
| out | The path to the directory where contracts are built | out | |
| interfaces | The path to the directory where the interfaces are located | src/interfaces | |
| contracts | The path to the directory where the contracts are located | src/contracts | |
| export_type | Export option which NPM package will be compatible | interfaces | interfaces, contracts |
Copy link
Member

@gas1cent gas1cent Oct 24, 2023

Choose a reason for hiding this comment

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

Let's sync the descriptions with what we have in the action.yml 🙏🏻

README.md Outdated
- name: Export Interfaces - ${{ matrix.export_type }}
uses: defi-wonderland/interface-exporter-action@v1
with:
package_name: '@defi-wonderland/interfaces-exporter-action-test-${{ matrix.export_type }}'
Copy link
Member

@gas1cent gas1cent Oct 24, 2023

Choose a reason for hiding this comment

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

Someone may be confused by the name of the package here, e.g. maybe it has to have interfaces-exporter in it. Let's make it obviously fake.

Suggested change
package_name: '@defi-wonderland/interfaces-exporter-action-test-${{ matrix.export_type }}'
package_name: 'your-project-${{ matrix.export_type }}'

README.md Outdated
export_type: '${{ matrix.export_type }}'

- name: Publish
run: cd export/@defi-wonderland/interfaces-exporter-action-test-${{ matrix.export_type }} && npm publish --access public
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run: cd export/@defi-wonderland/interfaces-exporter-action-test-${{ matrix.export_type }} && npm publish --access public
run: cd export/your-project-${{ matrix.export_type }} && npm publish --access public

@gas1cent gas1cent marked this pull request as ready for review October 24, 2023 06:15
@0xDiscotech 0xDiscotech requested a review from gas1cent October 24, 2023 15:35
README.md Outdated
run: yarn version --new-version "0.0.0-${GITHUB_SHA::8}" --no-git-tag-version

- name: Export Solidity - ${{ matrix.export_type }}
uses: defi-wonderland/interface-exporter-action@latest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok to use @latest?

Copy link
Member

Choose a reason for hiding this comment

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

@0xDiscotech I believe it will work only if we tag a release as latest. Not sure tbh, better specify @v1 or something like that. Also as a user, I'd prefer setting a known version to avoid the action stealing the NPM token or rugging me in a way, which is possible if you're always pulling the latest version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know! Will use @v1 so

@gas1cent gas1cent merged commit ac1cb20 into main Oct 24, 2023
@gas1cent gas1cent deleted the refactor/improve-readme branch October 24, 2023 17:41
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.

2 participants