talaia-labs / rust-teos

The Eye of Satoshi - Lightning Watchtower
https://talaia.watch
MIT License
128 stars 62 forks source link

Some changes came in mind so created pR. #201

Closed anmode closed 1 year ago

anmode commented 1 year ago

Here are the changes I made and why:

  1. Removed the fail-fast option from the build job's strategy: This is unnecessary since the default value is false, which is what we want.
  2. Combined the two toolchain options into one in the build job's matrix definition: This simplifies the definition and makes it easier to read.
  3. Added a exclude option to exclude the watchtower-plugin when building on Windows: The watchtower-plugin is not supported on Windows, so we need to exclude it from the build process on that platform.
  4. Upgraded to actions/checkout@v2: This is the latest version of the checkout action, and it's recommended to use it over previous versions.
  5. Renamed the Install Rust ${{ matrix.toolchain }} toolchainsteps to Setup Rust toolchain: This is a more accurate description of what the step is doing. Added a components option to the Setup Rust toolchain step in the lint job: This installs the rustfmt and clippycomponents, which are used in the linting process.
  6. Changed the cargo fmt command in the lint job to use the --check option: This checks the formatting of the code without actually changing it, which is more appropriate for a CI job. 5.Added a --denywarnings option to the cargo clippycommand in the lint job: This causes clippy to treat warnings as errors, which ensures that the code is free of potential issues.
sr-gi commented 1 year ago

Sorry but I don't think this PR makes sense, nor it is achieving what it is claiming to.

  1. Removed the fail-fast option from the build job's strategy: This is unnecessary since the default value is false, which is what we want.

This is not true. fail-fast defaults to true:

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast

  1. Combined the two toolchain options into one in the build job's matrix definition: This simplifies the definition and makes it easier to read.

Not sure what you're trying to achieve here, but with your approach all seems to be built using ubuntu-latest, which is certainly not what we're looking for.

  1. Added a exclude option to exclude the watchtower-plugin when building on Windows: The watchtower-plugin is not supported on Windows, so we need to exclude it from the build process on that platform.

This is exactly what was being done, but using a different syntax

  1. Upgraded to actions/checkout@v2: This is the latest version of the checkout action, and it's recommended to use it over previous versions.

How is v2 later than v3?

  1. Renamed the Install Rust ${{ matrix.toolchain }} toolchainsteps to Setup Rust toolchain: This is a more accurate description of what the step is doing.

I don't think this is more accurate

Added a components option to the Setup Rust toolchain step in the lint job: This installs the rustfmt and clippycomponents, which are used in the linting process.

This was already being done

  1. Changed the cargo fmt command in the lint job to use the --check option: This checks the formatting of the code without actually changing it, which is more appropriate for a CI job.

This was also already there, it was not introduced by this PR

5.Added a --denywarnings option to the cargo clippycommand in the lint job: This causes clippy to treat warnings as errors, which ensures that the code is free of potential issues.

Same as before, clippy was already denying warnings (using the exact option you're mentioning)

anmode commented 1 year ago

Oh okayyy @sr-gi got that.... Thanks and sorry If I misunderstood something.