Skip to content

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jan 24, 2025

  • Replaced deprecated io/ioutils with the equivalent calls
  • Fixed some linter complaints about errors format and other minor things
  • Removed useless libraries package at the top level, and merged the tests with the existing internal/libraries package.

Copy link

@dido18 dido18 left a comment

Choose a reason for hiding this comment

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

🧹

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

The libraries package was created for a reason:

#57

Please put it back.

@cmaglie
Copy link
Member Author

cmaglie commented Jan 27, 2025

The libraries package was created for a reason:

Thanks for the review @per1234!

Are we using this package somewhere? I could not find any reference to it.

If it's not used I'd prefer to remove it (even if it's technically a breaking change, I'm pretty sure nobody outside Arduino is using it).

If we are using I'd like to replace it with a package in the arduino-cli as we did for the fqbn, to reduce the spreading of the implementations.

@per1234
Copy link
Contributor

per1234 commented Jan 27, 2025

@per1234
Copy link
Contributor

per1234 commented Jan 27, 2025

I'd like to replace it with a package in the arduino-cli as we did for the fqbn, to reduce the spreading of the implementations.

It doesn't make sense to move it to the github.com/arduino/arduino-cli module because Arduino CLI doesn't use the registry file.

@cmaglie
Copy link
Member Author

cmaglie commented Jan 27, 2025

It doesn't make sense to move it to the github.com/arduino/arduino-cli module because Arduino CLI doesn't use the registry file.

I see.

What about moving validate-registry/main.go in a sub-command of libraries-repository-engine?

I think this would give us some long-term advantages:

  • the GitHub workflow in library-registry that checks the registry format would need to download the latest release of the library-repository-engine and run it on the registry.txt to check it, without the need to build it as a golang project.
  • it will simplify the library-registry repo since we could remove all the golang tooling assets
  • it will move all the business logic in a single place, which is easier to keep in sync

@per1234
Copy link
Contributor

per1234 commented Jan 27, 2025

What about moving validate-registry/main.go in a sub-command of libraries-repository-engine?

Sounds good.

@cmaglie cmaglie force-pushed the cleanups branch 2 times, most recently from 2484717 to 4a522c5 Compare January 28, 2025 11:43
@cmaglie cmaglie self-assigned this Jan 28, 2025
@cmaglie cmaglie added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jan 28, 2025
Copy link

@alessio-perugini alessio-perugini left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Thanks Cristian!

@cmaglie cmaglie merged commit a386e95 into main Jan 29, 2025
47 checks passed
@cmaglie cmaglie deleted the cleanups branch January 29, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants