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

remove explicit poi-shared-strings dependency #873

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

pjfanning
Copy link
Collaborator

@pjfanning pjfanning commented Jul 11, 2024

User description

  • spark-excel doesn't need poi-shared-strings
  • it's only used in excel-streaming-reader if certain features are enabled

PR Type

dependencies


Description

  • Removed the explicit poi-shared-strings dependency as it is not needed by spark-excel.
  • Upgraded the excel-streaming-reader dependency from version 4.3.1 to 5.0.1.

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 github-actions bot added the dependencies Pull requests that update a dependency file label Jul 11, 2024
    Copy link

    github-actions bot commented Jul 11, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 8f1ee0c)

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link

    github-actions bot commented Jul 11, 2024

    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]

    -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]

    -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
    Copy link
    Collaborator Author

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

    @pjfanning pjfanning closed this Jul 11, 2024
    Copy link

    Persistent review updated to latest commit 8f1ee0c

    @pjfanning
    Copy link
    Collaborator Author

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

    @nightscape nightscape merged commit 55978bc into main Jul 11, 2024
    60 checks passed
    @nightscape nightscape deleted the excel-streaming-reader-upgrade branch July 11, 2024 17:41
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    dependencies Pull requests that update a dependency file Review effort [1-5]: 1
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants