Skip to content

Array btreemap conversion #520

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

Closed

Conversation

kakserpom
Copy link
Contributor

@kakserpom kakserpom commented Jul 16, 2025

No description provided.

@coveralls
Copy link

coveralls commented Jul 16, 2025

Pull Request Test Coverage Report for Build 16353578993

Details

  • 0 of 18 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 24.494%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/types/array.rs 0 18 0.0%
Totals Coverage Status
Change from base Build 16353567988: -0.1%
Covered Lines: 968
Relevant Lines: 3952

💛 - Coveralls

@Xenira
Copy link
Collaborator

Xenira commented Jul 17, 2025

@kakserpom thank you <3

Just some things:

  1. Could you please use the MR template. It is there for a reason. No need to add a description when referencing an issue, but it serves as a reminder for both the author and the reviewer on what to check.
  2. Could you split this in 2 MRs, as these are 2 unrelated features
  3. Unit and integration tests are missing.
  4. At least the BTreeMap would benefit from being mentioned in the guides
  5. cargo fmt fails. We have git hooks that could catch that if you want to use them :)

@kakserpom kakserpom force-pushed the array_btreemap_conversion branch 2 times, most recently from d7f9aa4 to 085a547 Compare July 17, 2025 16:58
@kakserpom kakserpom changed the title Zval::null() and array btreemap conversion Array btreemap conversion Jul 17, 2025
@kakserpom
Copy link
Contributor Author

@Xenira Sure, now it's split into two MRs. I don't believe there's anything to test.

@Xenira Xenira force-pushed the array_btreemap_conversion branch from 085a547 to 83cb3de Compare July 17, 2025 18:58
Copy link
Collaborator

@Xenira Xenira left a comment

Choose a reason for hiding this comment

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

Think having unit and integration tests is an easy win here.

Can add them myself if you like.

Comment on lines +1294 to +1297
impl<'a, V> TryFrom<&'a ZendHashTable> for BTreeMap<String, V>
where
V: FromZval<'a>,
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do this here? Same for the others.

Suggested change
impl<'a, V> TryFrom<&'a ZendHashTable> for BTreeMap<String, V>
where
V: FromZval<'a>,
{
impl<'a, K, V> TryFrom<&'a ZendHashTable> for BTreeMap<K, V>
where
K: TryFrom<ArrayKey<'a>, Error = Error>,
V: FromZval<'a>,
{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Xenira Not sure this is a good idea.

@ptondereau
Copy link
Collaborator

hey! we split array.rs into smaller files. You can rebase and create a dedicated src/types/array/conversions/btreemap.rs

@kakserpom kakserpom closed this Jul 20, 2025
@kakserpom
Copy link
Contributor Author

#535

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.

4 participants