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

Fix Issue #295 #296

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Fix Issue #295 #296

merged 2 commits into from
Oct 3, 2023

Conversation

Pentiva
Copy link
Contributor

@Pentiva Pentiva commented Sep 14, 2023

This fixes issue #295 and does not fail any other tests, so I think this is probably the correct solution.
Whether this should be extended/limited in some way based on InterpreterOptions.LateBindObject, I'm not sure.

@davideicardi
Copy link
Member

Thank you @Pentiva!
I see tests are failing now because Path is not defined. I suggest to use just a static string.

Regarding the solution I don't see problem, but not sure if there is some edge case that I'm not aware. What do you think @metoule ?

Switched to string.Concat because Path.Combine changes might differ per OS
@Pentiva
Copy link
Contributor Author

Pentiva commented Sep 21, 2023

Sorry about that, I was using the online IDE github.dev for the first time and didn't notice it didn't add the usings automatically.
I think I'll stick to using my local computer for now.

Copy link
Member

@davideicardi davideicardi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@davideicardi davideicardi merged commit 2696232 into dynamicexpresso:master Oct 3, 2023
2 checks passed
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.

2 participants