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

Update doc for changes in wrappers #167

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

clostao
Copy link
Contributor

@clostao clostao commented Nov 12, 2024

User description

Changes in #161 were not reflected in the doc. This PR updates it


PR Type

Documentation


Description

  • Updated the README.md file to reflect recent changes in the auto-drive package, including new methods for file and folder uploads.
  • Added detailed usage examples for uploading files from different sources, including file paths and custom interfaces.
  • Introduced observables for tracking upload progress and provided examples for integrating with React.
  • Clarified the usage of the uploadFileFromFilepath, uploadFile, and uploadFolderFromFolderPath methods.
  • Explained how to handle uploads using rxjs observables and how to ignore them if not needed.

Changes walkthrough 📝

Relevant files
Documentation
README.md
Update documentation for file upload methods and observables

packages/auto-drive/README.md

  • Updated documentation to reflect changes in file upload methods.
  • Added detailed examples for uploading files and folders.
  • Introduced observables for tracking upload progress.
  • Provided examples for handling uploads in React and ignoring
    observables.
  • +171/-20

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

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    161 - Fully compliant

    Fully compliant requirements:

    • Update blake3 lib for @webuf/blake3 for enabling usage in browser
    • Add tools for uploading from browser (supporting File interface)
    • Add encrypted folder logic to this package (on encrypted folders upload a zip is generated and is uploaded as any encrypted file)
    • Add observability through rxjs package in downloads
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Documentation Clarity
    The documentation update introduces a lot of new methods and options which could potentially confuse users without proper context or simplified examples.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the function usage to match the imported function to prevent runtime errors

    Ensure that the uploadFile function is correctly imported and used since the code
    snippet shows uploadFileFromFilepath being imported but uploadFile being used. This
    might lead to a runtime error if uploadFile is not defined elsewhere.

    packages/auto-drive/README.md [41-50]

     import { uploadFileFromFilepath } from '@autonomys/auto-drive'
     ...
    -const uploadObservable = uploadFile(api, filePath, options)
    +const uploadObservable = uploadFileFromFilepath(api, filePath, options)
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a mismatch between the imported function and the one used in the code, which could lead to runtime errors. Fixing this is crucial for the functionality of the code.

    9
    Ensure that the buffer is initialized with actual data to avoid errors

    Replace the Buffer.from(...) with actual data or ensure that the placeholder is
    replaced with valid data to prevent runtime errors.

    packages/auto-drive/README.md [81]

    -const buffer = Buffer.from(...);
    +const buffer = Buffer.from('your actual data here');
    Suggestion importance[1-10]: 7

    Why: This suggestion is valid as it ensures that the buffer initialization is done with actual data, preventing potential runtime errors from a placeholder value.

    7
    Enhancement
    Implement error handling in the download function to manage potential exceptions effectively

    Add error handling for the downloadFile function to manage exceptions that may occur
    during the file download process, enhancing the robustness of the code.

    packages/auto-drive/README.md [182-186]

     const downloadFile = async (cid) => {
    -  const stream = await downloadObject(api, { cid })
    -  console.log('File downloaded successfully:', stream)
    +  try {
    +    const stream = await downloadObject(api, { cid })
    +    console.log('File downloaded successfully:', stream)
    +  } catch (error) {
    +    console.error('Error downloading file:', error)
    +  }
     }
    Suggestion importance[1-10]: 8

    Why: Adding error handling to the download function is a significant improvement for robustness, especially in network operations where errors are common.

    8
    Best practice
    Validate the progress value to ensure it falls within the expected range before updating the state

    Consider checking the progress value to ensure it is within a valid range (0-100)
    before setting it to state in the React example, to prevent UI errors or
    inconsistencies.

    packages/auto-drive/README.md [157]

    -setProgress(status.progress)
    +if (status.progress >= 0 && status.progress <= 100) {
    +  setProgress(status.progress)
    +}
    Suggestion importance[1-10]: 6

    Why: Validating the progress value is a good practice to maintain UI consistency and prevent errors, though it's not as critical as fixing functional bugs or errors.

    6

    Base automatically changed from fix/file-not-supported to main November 13, 2024 09:20
    @clostao clostao dismissed marc-aurele-besner’s stale review November 13, 2024 09:20

    The base branch was changed.

    @clostao
    Copy link
    Contributor Author

    clostao commented Nov 13, 2024

    I've updated the @autonomys/auto-dag-data doc too that I noticed it was outdated

    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.

    2 participants