search

nightscape / spark-excel

A Spark plugin for reading and writing Excel files
Apache License 2.0
469 stars 147 forks source link

remove explicit poi-shared-strings dependency #873

Closed pjfanning closed 4 months ago

pjfanning commented 4 months ago

User description

PR Type

dependencies

Description

Changes walkthrough 📝

Relevant files
Dependencies
build.sc
Remove poi-shared-strings and upgrade excel-streaming-reader
dependency
build.sc
  • Removed poi-shared-strings dependency.
  • Upgraded excel-streaming-reader dependency from version 4.3.1 to
    5.0.1.
    		•  +1/-2     

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    github-actions[bot] commented 4 months ago

    PR Reviewer Guide 🔍

    (Review updated until commit https://github.com/crealytics/spark-excel/commit/8f1ee0cd5405fc3b9b1c36ae364131b3b6c37b8a)

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review
    github-actions[bot] commented 4 months ago

    PR Code Suggestions ✨

    Latest suggestions up to 8f1ee0c

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Verify compatibility of the new excel-streaming-reader version with existing dependencies ___ **Ensure that the new version of excel-streaming-reader (5.0.1) is compatible with the
    other dependencies, especially since poi-shared-strings was removed. It's important
    to verify that this new version does not implicitly depend on a specific version of
    poi-shared-strings or any other library that might conflict with the existing
    dependencies.** [build.sc [93]](https://github.com/crealytics/spark-excel/pull/873/files#diff-c6f64a5895d9f518051cf83f8557f492ec6f5683a34c2914a46a894001e62726R93-R93) ```diff -ivy"com.github.pjfanning:excel-streaming-reader:5.0.1", +ivy"com.github.pjfanning:excel-streaming-reader:5.0.1", # Ensure compatibility with other dependencies ```
    Suggestion importance[1-10]: 8 Why: Ensuring compatibility of the new `excel-streaming-reader` version with existing dependencies is crucial to avoid runtime issues or conflicts, especially since `poi-shared-strings` was removed. This suggestion addresses a potential major issue.
    		8

    Previous suggestions

    Suggestions up to commit 279b31f
    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Check compatibility of the updated excel-streaming-reader with other project dependencies ___ **Ensure that the new version of excel-streaming-reader (5.0.0) is compatible with the
    other dependencies, especially since poi-shared-strings was removed. Compatibility
    issues could arise with major version changes.** [build.sc [93]](https://github.com/crealytics/spark-excel/pull/873/files#diff-c6f64a5895d9f518051cf83f8557f492ec6f5683a34c2914a46a894001e62726R93-R93) ```diff -ivy"com.github.pjfanning:excel-streaming-reader:5.0.0", +ivy"com.github.pjfanning:excel-streaming-reader:5.0.0", # Ensure compatibility with other dependencies ```
    Suggestion importance[1-10]: 8 Why: Ensuring compatibility when updating a dependency, especially with a major version change, is crucial to prevent potential runtime issues. The suggestion is valid and adds a useful comment for future maintainers.
    		8
    pjfanning commented 4 months ago

    problem in excel-streaming-reader - looks like it still has code that uses poi-shared-strings even in normal mode

    github-actions[bot] commented 4 months ago

    Persistent review updated to latest commit https://github.com/crealytics/spark-excel/commit/8f1ee0cd5405fc3b9b1c36ae364131b3b6c37b8a

    pjfanning commented 4 months ago

    excel-streaming-reader 5.0.1 seems to solve the issues I has with 5.0.0