-
Notifications
You must be signed in to change notification settings - Fork 5
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: ensure automatically generated filename is a valid filename #27
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
- Coverage 83.37% 82.87% -0.51%
==========================================
Files 10 10
Lines 1209 1226 +17
==========================================
+ Hits 1008 1016 +8
- Misses 141 145 +4
- Partials 60 65 +5 ☔ View full report in Codecov by Sentry. |
From a personal opinion, I wouldn't add a new dependency just for this. It's easier to just create a random string |
the why i try to avoid to just create a random string is that lots of other part of the code (legacy part) use that filename (extension part) so it breaks lots of things. |
@fmartingr some other place in code |
I didn't notice that dependency, was just doing some tests. If go-eupb already have it and it provides improvements over the standard library, yeah, absolutely. |
I check that again, just because in the current solution we don't check the actual byte of the file (check only the header), it doesn't have any specific advantage. |
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.
LGTM. I'd say this is a good compromise considering we are not downloading the files twice nor doing a big refactor in the code to guess the file type by its contents . 👍
some characters are not valid in some os like
?
in windows.we check filename and replace that kind of charachter with
-
so ``go-epub` always use valid filename in any os.Fix #26
@fmartingr can you please test this in shiori before we merge that too?
if it is ok i will just add more unit test to cover all characters in test too