Skip to content

Update MediaFile to fallback to current dir #1825

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

Open
wants to merge 2 commits into
base: canary
Choose a base branch
from

Conversation

gnpaone
Copy link

@gnpaone gnpaone commented Apr 20, 2025

This solution helps in situations of virtualized environment such as VSCode where file system provider cannot be determined. Solution for #1809


Important

Update MediaFile in media.rs to fallback to current directory when span_path parent is not resolvable, addressing issue #1809.

  • Behavior:
    • Update path() in MediaFile to fallback to current directory if span_path parent is not resolvable.
    • Handles cases in virtualized environments like VSCode where file system provider is undetermined.
  • Context:

This description was created by Ellipsis for 10b645e. You can customize this summary. It will automatically update as commits are pushed.

This solution helps in situations of virtualized environment such as VSCode where file system provider cannot be determined
Copy link

vercel bot commented Apr 20, 2025

@gnpaone is attempting to deploy a commit to the Gloo Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to 10b645e in 40 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/baml-types/src/media.rs:107
  • Draft comment:
    When falling back to current_dir(), add a brief comment explaining why this fallback is acceptable in virtualized environments and note potential pitfalls if used in production. Also consider logging a warning when the fallback is triggered.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
2. engine/baml-lib/baml-types/src/media.rs:107
  • Draft comment:
    Consider clarifying the error message. If both span_path.parent() and current_dir() fail, it might be clearer to indicate that a fallback (current directory) was attempted and also failed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. engine/baml-lib/baml-types/src/media.rs:107
  • Draft comment:
    Ensure that using the current directory as a fallback is acceptable in all environments. This behavior may lead to unexpected file resolution if the process's working directory differs from the intended base directory. Documentation on this behavior could help future maintainers.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_SPxkwMjHvLyta2o5

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@aaronvg
Copy link
Contributor

aaronvg commented Apr 21, 2025

was it hard to set this up? Were you able to test in the extension?

@aaronvg
Copy link
Contributor

aaronvg commented Apr 21, 2025

If you can post a screenshot of it working like in a dev container we'd be good to merge. We do have a function called findMediaFile in the typescript extension side, that talks to the VSCode extension to load a file, though if this fix works we may be okay with it as a fallback.

@hellovai
Copy link
Contributor

hellovai commented May 9, 2025

@gnpaone just wanted to follow up here on how you tested this so we can merge this in!

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.

3 participants