-
Notifications
You must be signed in to change notification settings - Fork 8
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
adapt singlecell example to macos #180
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
==========================================
- Coverage 60.07% 51.27% -8.81%
==========================================
Files 109 125 +16
Lines 7838 10733 +2895
==========================================
+ Hits 4709 5503 +794
- Misses 3129 5230 +2101 ☔ View full report in Codecov by Sentry. |
FILES=$(curl -s "$BASE_API" | grep -oP "\"path\": \"$FOLDER/.*?\.ibw\"" | grep -oP "(?<=\"$FOLDER/)[^\"]+") | ||
# Execute the following line if you are working on macos: | ||
# FILES=$(curl -s "$BASE_API" | grep -oE "\"path\": \"$FOLDER/.*?\.ibw") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should also work for linux, so we could even keep only this variant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked AI, and this is what it told me:
The -E option enables extended regular expressions, which is supported in GNU grep. However, the .*? lazy quantifier is not supported in extended regular expressions 1, 3.
To make this code work on a GNU Linux machine, you should use the -P option instead of -E to enable Perl-compatible regular expressions (PCRE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment confirms that just using grep -E on linux would not work because of the .*?
part, so we need to keep both the linux case and the macos case I believe: https://stackoverflow.com/questions/3027518/how-to-do-a-non-greedy-match-in-grep/5774431#5774431
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. What if we use the command below instead which I believe would give the same result and doesn't need PCRE:
FILES=$(curl -s "$BASE_API" | grep -oE "\"path\": \"$FOLDER/[^\"]*\.ibw" | grep -oE "[^/]*\.ibw$")
edited the command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, thanks. updated in latest commit with FILES=$(curl -s "$BASE_API" | grep -oE "\"path\": \"$FOLDER/[^\"]*\.ibw")
, as discussed offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approval please? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, all good! Thank you very much
No description provided.