-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Desktop: Resolves #6143: Add Installed Plugins in "About Joplin" #6357
Changes from 2 commits
71e1465
44dfbee
81ed5d3
408bc84
b5591d4
08f5fbc
e696aae
f65e91c
2ef6db8
124170c
146e83c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { _ } from './locale'; | ||
import { getPluginsArr } from './versionInfo'; | ||
import PluginService from '@joplin/lib/services/plugins/PluginService'; | ||
|
||
const plugins = getPluginsArr(PluginService.instance().plugins); | ||
|
||
describe('versionInfo', function() { | ||
|
||
it('should return an array', (()=>{ | ||
expect(Array.isArray(plugins)).toBe(true); | ||
})); | ||
|
||
it('should return an empty array', (() => { | ||
expect(plugins.length).toBe(0); | ||
})); | ||
|
||
it('should not push plugin to body if no plugin is found',(()=>{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is not really doing anything meaningful as it never calls any code that is used by Joplin. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you suggest any test case that I should Add? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Essentially you have a function from Plugins to string - this is all you need to test. When running in Joplin Plugins will be provided by the PluginService but in your test you can set the value manually and use it for testing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added New Tests |
||
const body = []; | ||
if (plugins.length > 0) { | ||
body.push(`\n${_('Plugins:')}`); | ||
plugins.map((plugin)=>{ | ||
body.push(`${plugin.name}: ${plugin.version}`); | ||
}); | ||
} | ||
expect(body.length).toBe(0); | ||
|
||
|
||
})); | ||
|
||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None of this feels very useful. getPluginsArr is just a "for" loop and we assume that loops work. What you should test instead is the versionInfo() function - make sure that when there are plugins, it includes them in the output of the function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, I'll add it. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,19 @@ | ||
import { _ } from './locale'; | ||
import Setting from './models/Setting'; | ||
import { reg } from './registry'; | ||
import PluginService, { Plugins } from '@joplin/lib/services/plugins/PluginService'; | ||
|
||
// Get Plugins in Array Form | ||
export const getPluginsArr = (pluginsObj: Plugins) => { | ||
const plugins = []; | ||
for (const pluginId in pluginsObj) { | ||
plugins.push({ | ||
name: pluginsObj[pluginId].manifest.name, | ||
version: pluginsObj[pluginId].manifest.version, | ||
}); | ||
} | ||
return plugins; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand the purpose of this function. You're looping on the array to create a new array. Then later you loop on this new array to create the plugin strings. So why not eliminate this intermediate step and loop on the first array to directly create the plugin strings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
export default function versionInfo(packageInfo: any) { | ||
const p = packageInfo; | ||
|
@@ -9,6 +22,8 @@ export default function versionInfo(packageInfo: any) { | |
gitInfo = _('Revision: %s (%s)', p.git.hash, p.git.branch); | ||
if (p.git.branch === 'HEAD') gitInfo = gitInfo.slice(0, -7); | ||
} | ||
|
||
|
||
const copyrightText = 'Copyright © 2016-YYYY Laurent Cozic'; | ||
const now = new Date(); | ||
|
||
|
@@ -32,6 +47,14 @@ export default function versionInfo(packageInfo: any) { | |
console.info(gitInfo); | ||
} | ||
|
||
const plugins = getPluginsArr(PluginService.instance().plugins); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From where versionInfo() is called, you already have the list of plugins in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if (plugins.length > 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just move this code inside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The gitInfo push to the body was down there so I thought to leave same types of items at a similar palace, also Should I change it? |
||
body.push(`\n${_('Plugins:')}`); | ||
plugins.map((plugin)=>{ | ||
body.push(`${plugin.name}: ${plugin.version}`); | ||
}); | ||
} | ||
|
||
return { | ||
header: header.join('\n'), | ||
body: body.join('\n'), | ||
|
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.
If you add return type annotation to
getPluginsArr
then this test becomes uselessThere 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 will remove the test and add types to the function