Skip to content

Return correct error instead of panicking if requested resolc version doesn't exist#23

Merged
pkhry merged 9 commits intomainfrom
pkhry/fix_126
May 21, 2025
Merged

Return correct error instead of panicking if requested resolc version doesn't exist#23
pkhry merged 9 commits intomainfrom
pkhry/fix_126

Conversation

@pkhry
Copy link

@pkhry pkhry commented May 13, 2025

return correct error when resolc version doesn't exist
Closes: paritytech/foundry-polkadot#126

@pkhry pkhry requested a review from smiasojed May 13, 2025 13:54
@smiasojed
Copy link
Collaborator

Could you please add the unit tests?

@smiasojed
Copy link
Collaborator

Please update the PR title to be more descriptive

@pkhry pkhry changed the title Fix issue #126 Return correct error instead of panicking if requested resolc version doesn't exist May 14, 2025
@pkhry
Copy link
Author

pkhry commented May 14, 2025

Could you please add the unit tests?

done

Please update the PR title to be more descriptive

done

let binary = versions.into_iter().next_back().expect("Can't be empty");
let Some(binary) = versions.into_iter().next_back() else {
let message = if let Some(v) = &_resolc_version {
format!("`resolc` v{v} doesn't exist")
Copy link
Collaborator

@smiasojed smiasojed May 14, 2025

Choose a reason for hiding this comment

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

when we set unknown version for solc we get:
Error: Unknown version provided
maybe we should update the error message to Error: Unknown solc version provided?
And for resolc
Error: Unknown resolc version provided
WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

when we set unknown version for solc we get: Error: Unknown version provided maybe we should update the error message to Error: Unknown solc version provided? And for resolc Error: Unknown resolc version provided WDYT?

  • Error: unknown version provided i can't find where this error is produced in code. do you have a link to it?
  • for solc we can have both autodetect and specific, so unsupported solc version can be added as a third message then. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not have it, I just see such message when using unknown solc version

Copy link
Author

Choose a reason for hiding this comment

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

I do not have it, I just see such message when using unknown solc version

i can both come from svm-rs and rvm-rs

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that it does not make sense to change svm or rvm now

@pkhry pkhry requested a review from smiasojed May 15, 2025 08:36
@pkhry pkhry requested a review from filip-parity May 21, 2025 08:01
@pkhry pkhry merged commit 4f1d461 into main May 21, 2025
16 checks passed
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.

[Bug] Panic when used not existing compiler

2 participants