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

Handle saved Windows paths on Unix #100

Closed
wants to merge 6 commits into from

Conversation

paulfd
Copy link
Member

@paulfd paulfd commented Sep 11, 2023

No description provided.

@paulfd paulfd force-pushed the paulfd/file-search-win-path-fix branch from b17a0c6 to 028fc90 Compare September 11, 2023 05:32
Copy link
Member

@redtide redtide left a comment

Choose a reason for hiding this comment

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

Maybe the solution is a "create and upload artifacts only when tagging a release or on pull requests, to keep the builds as minimum and fast as possible when just pushing a commit"

Also as self reminder we can omit the ${{ in if conditionals as stated in the docs:

[...] you may omit the ${{ }} expression syntax because GitHub Actions automatically evaluates the if conditional as an expression [...]
You must use the ${{ }} expression syntax or escape with '', "", or () when the expression starts with !, since ! is reserved notation in YAML format.
Using the ${{ }} expression syntax turns the contents into a string, and strings are truthy. For example, if: true && ${{ false }} will evaluate to true.

so these may be
if: github.ref_type == 'tag' || github.event_name == 'pull_request'

@atsushieno
Copy link
Contributor

atsushieno commented Sep 20, 2023

Assuming it is a fix for #95, I created another fix locally (noticed this one only after I creaeted mine) which is to replace \ with / only on Windows.

If this fix is going to be good and applied it's great, but note that \ is a valid filename character on various Linux filesystems) so you cannot simply replace \ without detecting the platform anyways.

Also, there are paths like "\server\mypc\sfz\foobar.sfz" on Windows, so path[1] is not always :. Maybe fs::path::is_absolute() is good enough?

@redtide redtide force-pushed the paulfd/file-search-win-path-fix branch 4 times, most recently from 8ed66e6 to d4333f7 Compare September 20, 2023 23:21
@redtide redtide force-pushed the paulfd/file-search-win-path-fix branch from d4333f7 to fa03059 Compare September 21, 2023 01:29
@redtide
Copy link
Member

redtide commented Oct 23, 2023

@paulfd what is the status? Should this be closed and merge #101 instead, adding the "create artifacts also on PR" feature on it?

@redtide
Copy link
Member

redtide commented Nov 23, 2023

Closing, #101 merged instead.

@redtide redtide closed this Nov 23, 2023
@redtide redtide deleted the paulfd/file-search-win-path-fix branch February 23, 2024 13:00
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