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

bug fixing FTP url #38

Merged
merged 3 commits into from
Oct 1, 2024
Merged

bug fixing FTP url #38

merged 3 commits into from
Oct 1, 2024

Conversation

ypriverol
Copy link
Contributor

@ypriverol ypriverol commented Oct 1, 2024

PR Type

bug_fix, documentation


Description

  • Fixed the FTP URL in the Files class by removing the 'ftp://' prefix.
  • Updated the README to use a variable for the version in the installation command.
  • Corrected a minor formatting issue in the README note regarding globus URLs.

Changes walkthrough 📝

Relevant files
Bug fix
files.py
Fix FTP URL in Files class                                                             

pridepy/files/files.py

  • Corrected the FTP URL by removing the 'ftp://' prefix.
+1/-1     
Documentation
README.md
Update installation command and fix formatting                     

README.md

  • Updated installation command to use a variable for version.
  • Fixed minor formatting issue in a note.
  • +2/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Bug
    The removal of 'ftp://' prefix from PRIDE_ARCHIVE_FTP might cause issues if the variable is used in code that expects a full URL.

    Documentation Improvement
    The installation command now uses a placeholder {version}, which might confuse users if not explained.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Oct 1, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Provide a concrete way to determine the package version for installation

    Replace the placeholder {version} with an actual version number or provide
    instructions on how to determine the correct version number to use.

    README.md [35]

    -$ pip install dist/pridepy-{version}.tar.gz
    +$ pip install dist/pridepy-$(python setup.py --version).tar.gz
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances usability by providing a clear method to determine the package version, reducing potential confusion during installation.

    8
    ✅ Provide more context about the use of HTTPS instead of the Globus protocol

    Consider adding a brief explanation or link to documentation about the Globus
    protocol and why HTTPS is used instead. This would provide more context for users
    who might be unfamiliar with Globus.

    README.md [58]

    ->**NOTE**: Currently we use globus urls (when -p globus is used) via https not globus protocol.
    +>**NOTE**: Currently we use Globus URLs (when `-p globus` is used) via HTTPS, not the Globus protocol. For more information about Globus, see [Globus documentation](https://www.globus.org/data-transfer).

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    Why: Adding context and a link to the Globus documentation helps users understand the choice of protocol, improving the documentation's clarity and usefulness.

    7
    Maintainability
    Use a constant for the FTP protocol to improve maintainability

    Consider using a constant or configuration variable for the FTP protocol. This
    allows for easier management if the protocol needs to be changed in the future, and
    improves code readability.

    pridepy/files/files.py [55]

    -PRIDE_ARCHIVE_FTP = "ftp.pride.ebi.ac.uk"
    +FTP_PROTOCOL = "ftp"
    +PRIDE_ARCHIVE_FTP = f"{FTP_PROTOCOL}.pride.ebi.ac.uk"
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to use a constant for the FTP protocol improves maintainability and readability, but it is not critical since the protocol is unlikely to change frequently.

    5

    💡 Need additional feedback ? start a PR chat

    README.md Outdated Show resolved Hide resolved
    Co-authored-by: codiumai-pr-agent-pro[bot] <151058649+codiumai-pr-agent-pro[bot]@users.noreply.github.com>
    @ypriverol ypriverol merged commit c026e16 into master Oct 1, 2024
    6 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant