Skip to content

Conversation

@Eagle3386
Copy link
Contributor

Fixes wrong RegEx as mentioned in corresponding deploy API issue.

Really don't know, how that'd slipped through, because I wrote the correct RegEx just a couple of comments above the linked one… 🙈😅

Anyway, this fixes the issue once & for all. 😎

@Eagle3386
Copy link
Contributor Author

@Neilpang I don't know why GitHub states "branch conflict" for simply changing the RegEx range from 0-9 to a-z.… 🤔

@winromulus
Copy link
Contributor

@Eagle3386 PR #4809 was already merged. Probably that's why you get conflicts.

@winromulus
Copy link
Contributor

@Eagle3386 considering Synology's naming conventions, I wouldn't assume that the entry will not contain any numeric or other characters in the path. Ideally this would have been picked up using jq instead of sed.

@Eagle3386
Copy link
Contributor Author

Eagle3386 commented Sep 27, 2023

@Eagle3386 PR #4809 was already merged. Probably that's why you get conflicts.

Probably not,because if it wasn't, my PR would still change the exact same RegEx range.
If it wasn't merged already that'd make sense, but it is, so…

@Eagle3386 considering Synology's naming conventions, I wouldn't assume that the entry will not contain any numeric or other characters in the path. Ideally this would have been picked up using jq instead of sed.

I've never come across an API endpoint of them that uses numbers. Other chars might be, but again, never seen that up to now.
Using jq would've been an option, yes - especially, since the file header even mentions it as a requirement.
However, neither the original script nor my update actually used it before & so I opted to follow the current way. That's because AFAIK jq isn't available by default on all platforms & Neil's requirement is to write cross platform compatible code, using "standard tools".

However, I'm fine with ^" as RegEx selector instead of my attempt with a-z., so closing this PR now.

@Eagle3386 Eagle3386 closed this Sep 27, 2023
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