Skip to content

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
Member

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
Member

@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
Member

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
Member

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