-
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
Clean up code (drop unreachable code) and minor modernization #565
Conversation
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.
For easier review, left some comments
if (file.substring(file.length - 3) === 'php' && !isCore) { | ||
link += '/index.php/apps/' + app | ||
if (isPHP && !isCore) { | ||
link += `/index.php/apps/${app}` |
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.
template strings are more performant on modern browsers than string concatenation (and I think better readable).
} | ||
if (link.substring(link.length - 1) !== '/') { | ||
if (link.at(-1) !== '/') { |
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.
.at
supports negative indices (like in most other languages) which are the index from the end, meaning -1 is the last char.
Reduces the complexity of this statement.
if (!isCore) { | ||
link += 'apps/' | ||
} |
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 could never reached, because the above if-else were covering !isCore && isPHP
and !isCore && !isPHP
meaning when we end here isCore
is always true.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #565 +/- ##
==========================================
+ Coverage 97.53% 98.73% +1.20%
==========================================
Files 1 1
Lines 243 238 -5
Branches 36 34 -2
==========================================
- Hits 237 235 -2
+ Misses 5 1 -4
- Partials 1 2 +1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
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.
Couldn't find nothing wrong or broken, seem to be a good patch)
Only tests are missing, right?
@@ -148,43 +148,38 @@ export const imagePath = (app: string, file: string) => { | |||
* @return {string} URL with webroot for a file in an app | |||
*/ | |||
export const generateFilePath = (app: string, type: string, file: string) => { | |||
const isCore = window?.OC?.coreApps?.indexOf(app) !== -1 | |||
const isCore = window?.OC?.coreApps?.includes(app) | |||
const isPHP = file.slice(-3) === 'php' |
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 is also equivalent to file.substring(file.length - 3) === 'php'
, good catch to reuse it
link += '/index.php/' | ||
} else { | ||
link += '/' | ||
link += '/index.php' |
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.
Moving a /
character a bit seems to be correct, and isolated in the else
block
Yes added the missing test for when the |
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
No description provided.