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

Add more specific err message for invalid struct #164

Merged
merged 14 commits into from
May 29, 2024

Conversation

cryptopapi997
Copy link
Contributor

@cryptopapi997 cryptopapi997 commented May 22, 2024

Follow up from #160 , where the issue was camel -> kebab -> camel case conversion isn't always constant. This was hard to determine since trident didn't say which struct was the problem. Saying which one it was (and seeing that it sees it incorrectly capitalized) would've made this easy to debug, so this PR changes this.

Maybe trident not supporting vars with multiple sequential capital letters could be specified in the docs somewhere too, but wasn't sure where the best spot for this is, so up to you guys.

@lukacan
Copy link
Collaborator

lukacan commented May 22, 2024

Hello @cryptopapi997 , thanks for the PR , we will have a look and get back to you later :)

@lukacan lukacan requested review from Ikrk and lukacan May 22, 2024 14:08
@@ -545,7 +547,9 @@ pub fn parse_to_idl_program(name: String, code: &str) -> Result<IdlProgram, Erro
};
Some(fields.into_iter())
})
.ok_or(Error::MissingOrInvalidProgramItems("instruction struct"))?;
.ok_or(Error::MissingOrInvalidProgramItems(
"instruction struct".to_string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about also including the "instruction_struct_name" here?

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 point! Just added it

@cryptopapi997
Copy link
Contributor Author

Adapted according to your review, feel free to take another look @lukacan

@Ikrk Ikrk changed the base branch from master to develop May 29, 2024 11:00
@Ikrk Ikrk changed the base branch from develop to master May 29, 2024 11:02
@Ikrk
Copy link
Contributor

Ikrk commented May 29, 2024

Hi @cryptopapi997, thank you, it looks good to me!
Can you please rebase your branch on develop and change this PR to merge to develop branch? We are currently pushing new features into develop branch and merge to master only on release.

We might however reconsider this branching strategy in the (short) future in order to make contributions easier...

@cryptopapi997 cryptopapi997 changed the base branch from master to develop May 29, 2024 11:26
@cryptopapi997
Copy link
Contributor Author

Got it, just rebased & resolved merge conflicts - feel free to take a final look @Ikrk

@Ikrk Ikrk added the no changelog Lable to skip `check changelog` in CI pipeline. label May 29, 2024
@Ikrk Ikrk merged commit 36664f1 into Ackee-Blockchain:develop May 29, 2024
8 of 9 checks passed
@Ikrk
Copy link
Contributor

Ikrk commented May 29, 2024

Thanks, merged!

lukacan added a commit that referenced this pull request Jul 10, 2024
* Update README.md

updated features

* 📝 update urls in readme

* 📌 bump crate versions

* Update README.md

added break

* 🚑️ update docs before release

* 📝 fix discord link

* 📝 (add mike): Add mike

* add more specific err message

* add missing name on instruction struct too

* Update index.md

* Update index.md

* rm main.html

---------

Co-authored-by: Adam Hrazdira <[email protected]>
Co-authored-by: Emre Ekinci <[email protected]>
Co-authored-by: lukacan <[email protected]>
lukacan added a commit that referenced this pull request Jul 10, 2024
* Update README.md

updated features

* 📝 update urls in readme

* 📌 bump crate versions

* Update README.md

added break

* 🚑️ update docs before release

* 📝 fix discord link

* 📝 (add mike): Add mike

* add more specific err message

* add missing name on instruction struct too

* Update index.md

* Update index.md

* rm main.html

---------

Co-authored-by: Adam Hrazdira <[email protected]>
Co-authored-by: Emre Ekinci <[email protected]>
Co-authored-by: lukacan <[email protected]>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Lable to skip `check changelog` in CI pipeline.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants