near / bos-loader

MIT License
19 stars 5 forks source link

Refactor -- General cleanup/Async/Error Messaging #21

Closed robertlopezdev closed 4 months ago

robertlopezdev commented 5 months ago

Hello,

During a recent discussion this repo was mentioned, so I explored refactoring. The key changes include:

I have ensured that all unit tests pass and that the output from the test/components is consistent with the original version's output.

This refactor introduces dependencies on anyhow and async_recursion to facilitate the transition to async. If these additions are a concern, I am open to discussing alternatives or reverting to the initial synchronous rewrite I committed, which still offers improved error messaging, syntax and some cleaning up of un-needed cloning.

Open to any discussions or requests for changes. Looking forward to your thoughts!

Best regards,

Robert Lopez

robertlopezdev commented 5 months ago

awesome! I have been hoping for an improvement sweep like this to take place

One regression that was not accounted for in unit tests: in config files, the property expected is now account_id instead of account

You can reproduce by running with cargo run -- -c in the test dir

Did not catch that, thanks.

I made a commit to update this and to update the README to reflect the change as well.

mpeterdev commented 4 months ago

awesome! I have been hoping for an improvement sweep like this to take place One regression that was not accounted for in unit tests: in config files, the property expected is now account_id instead of account You can reproduce by running with cargo run -- -c in the test dir

Did not catch that, thanks.

I made a commit to update this and to update the README to reflect the change as well.

Sorry for not making this clearer: since this is a tool that is already widely used, we should resolve the regression and continue looking for account in the config file instead of changing to account_id there. Otherwise we are shipping a breaking change to users without good reason

robertlopezdev commented 4 months ago

awesome! I have been hoping for an improvement sweep like this to take place One regression that was not accounted for in unit tests: in config files, the property expected is now account_id instead of account You can reproduce by running with cargo run -- -c in the test dir

Did not catch that, thanks. I made a commit to update this and to update the README to reflect the change as well.

Sorry for not making this clearer: since this is a tool that is already widely used, we should resolve the regression and continue looking for account in the config file instead of changing to account_id there. Otherwise we are shipping a breaking change to users without good reason

Gotcha, reverted back to account.

mpeterdev commented 4 months ago

thanks for you contribution @robertlopezdev !