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

Upgrade dependencies #9

Merged
merged 2 commits into from
Sep 29, 2024
Merged

Upgrade dependencies #9

merged 2 commits into from
Sep 29, 2024

Conversation

Swoorup
Copy link
Owner

@Swoorup Swoorup commented Sep 29, 2024

No description provided.

Copy link

gooroo-dev bot commented Sep 29, 2024

Please double check the following review of the pull request:

Issues counts

🐞Mistake 🤪Typo 🚨Security 🚀Performance 💪Best Practices 📖Readability ❓Others
0 0 0 0 1 0 0

Changes in the diff

  • ➕ Added version "0.7.0" to the main Cargo.toml.
  • ✅ Upgraded arrow dependency from "52.0" to "53.0".
  • ✅ Upgraded half dependency from "2.1" to "2.4".
  • ✅ Upgraded proc-macro-error dependency to proc-macro-error2 version "2".
  • 🛠️ Changed version, authors, edition, and license fields to workspace settings in arrow_convert and arrow_convert_derive Cargo.toml files.
  • 🛠️ Updated imports from proc_macro_error to proc_macro_error2 in arrow_convert_derive source files.

Identified Issues

ID Type Details Severity Confidence
1 💪Best Practices The version change for proc-macro-error to proc-macro-error2 should be verified for compatibility with existing code. 🟠Medium 🟠Medium

Issue 1: Compatibility Check for proc-macro-error2

Explanation

The upgrade from proc-macro-error to proc-macro-error2 should be verified for compatibility with existing code. This change affects multiple files and could potentially introduce breaking changes if the new version has different behavior or API.

Code to Address the Issue

No specific code change is required, but a thorough review and testing of the affected code should be performed to ensure compatibility.

Explanation of the Fix

Ensure that all usages of proc-macro-error are correctly updated to proc-macro-error2 and that the new version does not introduce any breaking changes. This can be achieved by running existing tests and adding new tests if necessary.

Missing Tests for Incoming Changes

To ensure the changes are correctly integrated and do not introduce any issues, the following tests should be added:

  1. Test for arrow_convert and arrow_convert_derive versioning:

    • Ensure that the versioning is correctly set to workspace settings and that it does not affect the functionality.
  2. Test for proc-macro-error2 integration:

    • Add tests to verify that all functionalities using proc-macro-error are correctly working with proc-macro-error2.
  3. Test for upgraded dependencies:

    • Ensure that the upgraded dependencies (arrow, half, proc-macro-error2) do not introduce any breaking changes or regressions.

Here is an example of a basic test for proc-macro-error2 integration:

#[cfg(test)]
mod tests {
    use super::*;
    use proc_macro_error2::{abort, proc_macro_error};

    #[test]
    #[proc_macro_error]
    fn test_proc_macro_error2_integration() {
        // Example test to ensure proc_macro_error2 is working as expected
        let result = std::panic::catch_unwind(|| {
            abort!(Span::call_site(), "This is a test error");
        });
        assert!(result.is_err(), "Expected an error but got none");
    }
}

This test ensures that the proc_macro_error2 integration is working as expected by triggering an error and verifying that it is correctly caught. Similar tests should be added for other functionalities affected by the dependency upgrades.

Summon me to re-review when updated! Yours, Gooroo.dev
Feel free to react or reply to this review!

Copy link

codecov bot commented Sep 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.88%. Comparing base (9d90aef) to head (6386225).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #9      +/-   ##
==========================================
- Coverage   93.11%   92.88%   -0.23%     
==========================================
  Files           9        9              
  Lines        1743     1743              
==========================================
- Hits         1623     1619       -4     
- Misses        120      124       +4     
Flag Coverage Δ
92.88% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Swoorup Swoorup merged commit 8895362 into main Sep 29, 2024
5 checks passed
@Swoorup Swoorup deleted the sj-upgrade-arrow branch October 1, 2024 01:28
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.

1 participant