Initial github-actions configuration #1
Conversation
danigm
changed the title
danigm
force-pushed
the
ci
branch
17 times, most recently
from
a910459 to
889c4a9
Compare
danigm
changed the title
danigm
force-pushed
the
ci
branch
3 times, most recently
from
b0aaeee to
a7782e6
Compare
There was a problem hiding this comment.
The 1st commit looks great, although I'm not a CI expert. It would be great if someone with experience like @dbnicholson could take a look at it. Otherwise feel free to merge this commit.
In the 2nd commit I see changes for things that were passing the linter in the kolibri branch. I only mentioned one of them, but there are many. I think it would be much better to have the same lint rules as the core plugins, unless we come up with a good reason not to do so. Hypothetically we could be upstreaming this plugin in the future.
| import KolibriApp from 'kolibri_app'; | ||
| import RootVue from './views/ExploreIndex'; | ||
| import routes from './routes'; | ||
| import { setFacilitiesAndConfig } from './modules/coreExplore/actions'; | ||
| import pluginModule from './modules/pluginModule'; | ||
| import KolibriApp from 'kolibri_app'; |
There was a problem hiding this comment.
Why is the lint complaining about this? Are the rules different than the ones for the kolibri core plugins? If so it would be nice to use the same rules as core plugins, unless we have a reason for doing things differently.
There was a problem hiding this comment.
I've copied all config files for linting from kolibri project, but there should be something that I'm missing because the lint is different. It uses the kolibri-tools for the linting. I'll review and take a look, let's see if I find where is the difference.
There was a problem hiding this comment.
I've been looking into this issue and we're using the same configuration than we've in kolibri. I think this import order change is done because in the external plugin app, the kolibri_app is an external import (from kolibri-tools), and as a plugin is internal.
That's why this is reordered by the linting. In any case, I'll split all linting changes in different commits so it's easier to review.
There was a problem hiding this comment.
Ah, I see. Sorry then! That explains this change but not the other changes. I don't see the need to split the changes, but you already did the work so it's great. Let's merge it if you think all these lint changes are necessary now.
There was a problem hiding this comment.
Looks pretty good to me. I'm honestly a github workflow noob myself, but the way you're driving it looks appropriate. I was wondering how node was getting in there from a python setup and had not been aware of nodeenv. You could also use the setup-node action, but if you're already using nodeenv, then so be it.
As an external module kolibri.core.* module is external so the import line should not be split from other externals. https://phabricator.endlessm.com/T31232
kolibri_app is part of kolibri-tools and as external module this is an external import so it should be placed just below the router and above the internals imports. This is different from what's required for internal plugins because in that case, kolibri_app is part of the kolibri core project. This patch also adds an exception for this class to do not require the use of this on methods. https://phabricator.endlessm.com/T31232
This patch replaces the use of undefined with null. https://eslint.org/docs/rules/no-undefined https://phabricator.endlessm.com/T31232
This patch fix the lint on the showChannels methods, adding a return statement for all the cases. To simplify the code we use here an early-return, if there's no node we return null, so we can remove one if deep here. https://eslint.org/docs/rules/consistent-return https://eslint.org/docs/rules/array-callback-return https://phabricator.endlessm.com/T31232
'showCustomContent' and 'showTopicsTopic' was used before it was defined. This patch reorders the function declaration. https://eslint.org/docs/rules/no-use-before-define https://phabricator.endlessm.com/T31232
This patch adds some exeptions to the no-empty-function rule for default values, and it's also removed a catch that does nothing. https://eslint.org/docs/rules/no-empty-function https://phabricator.endlessm.com/T31232
Use a single literal string instead a concatenation. This patch also renames the nodes variable to avoid shadowing. https://eslint.org/docs/rules/no-useless-concat https://eslint.org/docs/rules/no-shadow https://phabricator.endlessm.com/T31232
| import KolibriApp from 'kolibri_app'; | ||
| import RootVue from './views/ExploreIndex'; | ||
| import routes from './routes'; | ||
| import { setFacilitiesAndConfig } from './modules/coreExplore/actions'; | ||
| import pluginModule from './modules/pluginModule'; | ||
| import KolibriApp from 'kolibri_app'; |
There was a problem hiding this comment.
Ah, I see. Sorry then! That explains this change but not the other changes. I don't see the need to split the changes, but you already did the work so it's great. Let's merge it if you think all these lint changes are necessary now.
https://phabricator.endlessm.com/T31232