[WIP]Support GitLab #44

bekicot · 2018-06-07T05:00:38Z

No description provided.

jayvdb · 2018-06-07T05:03:12Z

No code can be merged until the license issue is resolved.

blazeu · 2018-06-08T05:53:12Z

app/controllers/tasks.js

@@ -4,4 +4,5 @@ import {inject} from '@ember/service';
 export default Controller.extend({
  organizations: inject(),
  queryParams: ['org'],
+


Irrelevant diff

blazeu · 2018-06-08T05:55:08Z

app/templates/tasks.hbs

+          {{#link-to (query-params org=slug)}}
+          {{org.name}}
+          {{/link-to}}
+        </li>


Keep the indent?

Nope, i will change. That's silly mistake

bekicot · 2018-06-09T06:37:56Z

@jayvdb
The fixes has been created. I've made requested PR #42 #47
This PR, ready to merge

andrewda · 2018-06-09T06:41:36Z

app/utils/task-utils.js

@@ -0,0 +1,9 @@
+export const getTrackerIdentifiers = (org, trackerType) => {
+  if(org && org.trackers){


Space before ( and after )

andrewda · 2018-06-09T06:43:12Z

app/services/user-settings.js

+    const settings = getObject(SETTINGS_KEY) || {};
+
+    // Only set property defined in PROPERTIES
+    for(let property of PROPERTIES) {


You've got some for loops with a space between the for and the opening parenthesis, and some without. Should be fixed with the .coafile, but I believe we usually go with a space.

@bekicot You should've added style rules to .eslintrc like I said

jayvdb · 2018-06-11T13:54:28Z

app/routes/tasks/gitlab.js

+  store: inject(),
+  model() {
+    const store = this.get('store');
+    return this.get('gitlab').tasks({projects: ['coala/mobans']}).then(data => {


why does this have a coala specific project reference in here?

jayvdb · 2018-06-11T14:16:47Z

app/templates/tasks.hbs

@@ -13,7 +13,7 @@
        Organizations
      </p>
      <ul class="menu-list">
-        {{#each-in organizations.list as |slug org| }}
+        {{#each-in organizations.list as |slug org|}}


this should be fix in the first PR, if it is important
see #33 (comment)

jayvdb · 2018-06-11T14:17:26Z

app/services/gitlab.js

+  _issueUrl: '',
+  init() {
+    this._super(...arguments);
+    this.set('headers', {'Private-Token':  'sVfZzagWtemrV-suxYK-'});


this shouldnt be here ... ?

blazeu · 2018-06-12T08:10:14Z

app/services/gitlab.js

+    let tasks = projects.map((projectId) => {
+      const embedRepoInfo = (tasks) => {
+        return new Promise((resolve, reject) => {
+          this.fetchRepositoryInfo(projectId).then((repoInfo) => {


A promise can be chained, no need to explicitly make Promise

blazeu · 2018-06-12T08:11:37Z

app/services/gitlab.js

+      const embedRepoInfo = (tasks) => {
+        return new Promise((resolve, reject) => {
+          this.fetchRepositoryInfo(projectId).then((repoInfo) => {
+            tasks.map((task) => {


if you return tasks.map the result will be in the resolved Promise.

blazeu · 2018-06-12T08:12:17Z

app/services/gitlab.js

+              task.type = 'gitlab-task';
+              return task;
+            })
+            resolve(tasks)


And this doesn't return the mapped tasks because map creates new array not modify it.

Actually it does, but .forEach is more appropriate than .map

blazeu · 2018-06-12T08:21:46Z

app/serializers/gitlab-task.js

+    task.repository.nameWithOwner = payload.repository.path_with_namespace
+    task.repository.url = payload.repository.web_url
+
+    task.isPullRequest = payload._type == 'PullRequest';


andrewda

There's still quite a few styling things that would be easily caught and fixed with eslint

andrewda · 2018-06-13T16:20:41Z

app/components/task-item.js

+      formatHref: function (href, type) {
+        if (type === 'mention') {
+          href = 'https://github.com/' +
+            href.substring(1);


This shouldn't be over the limit, why split into multiple lines?

No reason, you are right.

andrewda · 2018-06-13T16:22:21Z

app/components/task-item.js

+      validate: {
+        url: function (value) {
+          return /^(http|ftp)s?:\/\//.test(value);
+        },


Can be a pretty arrow function:

url: (value) => /^(http|ftp)s?:\/\//.test(value),

andrewda · 2018-06-13T16:23:25Z

app/components/task-item.js

+          return /^(http|ftp)s?:\/\//.test(value);
+        },
+      },
+      formatHref: function (href, type) {


This can also be an arrow function. Doesn't have much benefit besides consistency, but that's pretty important.

andrewda · 2018-06-13T16:23:54Z

app/controllers/tasks.js

+export default Controller.extend({
+  organizations: inject(),
+  queryParams: ['org'],
+


Unnecessary newline?

andrewda · 2018-06-13T16:25:12Z

app/graphql/github.js

+
+export const queryBuilder = function queryBuilder({orgs}) {
+  let queries = ['sort:updated-desc', 'state:open'];
+  if(isIterable(orgs)) {


Space after if (at least that's how you've been doing it everywhere else). Not sure what the progress is yet on an eslint?

andrewda · 2018-06-13T16:26:53Z

app/routes/application.js

-    controller.set('organizations', organizationList);
-    controller.set('searchParams', model.q)
+    // show modal when user has no github token
+    if(!this.userSettings.get('githubTokenModalSeen'))


space after if

andrewda · 2018-06-13T16:27:52Z

app/services/tasks.js

@@ -0,0 +1,3 @@
+import Service from '@ember/service';
+export default Service.extend({
+});


Doesn't match formatting of similar files

andrewda · 2018-06-13T16:28:13Z

app/utils/storage.js

@@ -0,0 +1,12 @@
+export const setObject = function(key, value) {
+  localStorage.setItem(key, JSON.stringify(value));
+};


Arrow function

andrewda · 2018-06-13T16:28:22Z

app/utils/storage.js

+  localStorage.setItem(key, JSON.stringify(value));
+};
+
+export const getObject = function(key) {


Arrow function

andrewda · 2018-06-13T16:29:10Z

app/routes/tasks/github.js

+
+    let tasks = github.tasks({orgs: identifiers});
+    tasks.then((data) => {
+      data.forEach(function(task) {


This should definitely be an arrow function

blazeu · 2018-07-11T16:01:33Z

app/controllers/tasks.js

@@ -4,4 +4,5 @@ import {inject} from '@ember/service';
 export default Controller.extend({
  organizations: inject(),
  queryParams: ['org'],
+


Irrelevant diff

blazeu · 2018-07-11T16:05:28Z

app/services/gitlab.js

@@ -0,0 +1,50 @@
+// https://gitlab.com/api/v4/projects/coala%2Fmobans/issues?order_by=created_at&state=opened


What's this comment?

blazeu · 2018-07-11T16:06:18Z

app/services/gitlab.js

+  _issueUrl: '',
+  init(...arg) {
+    this._super(...arg);
+    this.set('headers', { 'Private-Token': 'sVfZzagWtemrV-suxYK-' });


This shouldn't be here?

This was mentioned a month ago at #44 (comment)

jayvdb · 2018-07-12T15:23:33Z

app/services/gitlab.js

+  _issueUrl: '',
+  init(...arg) {
+    this._super(...arg);
+    this.set('headers', { 'Private-Token': 'sVfZzagWtemrV-suxYK-' });


This was mentioned a month ago at #44 (comment)

jayvdb · 2018-07-12T15:27:35Z

app/data/organizations.js

@@ -11,7 +11,7 @@ export default {
      },
      {
        type: 'gitlab',
-        identifier: 'coala',
+        identifier: 'coala/mobans',


This is obviously wrong, as it is not an org identifier. It is a project identifier.

This is not too different from the problem I raised in the PR a month ago.
#44 (comment)

fetchGitlabProjects still doesnt get the projects for the org.

Identifier in gitlab currently only support per repository, however, in github, only support per organization.

In gitlab, supporting per org, will make multiple request.

It fit nicely with wikidata where, it already support github organization, but not gitlab. It can be identified via issue tracker used, which is currently support full link.

Move GitHub Tasks to github namespace `/tasks/github` Add a new gitlab route `/tasks/gitlab` Add a new gitlab serializer Closes coala#38

bekicot changed the title ~~Support GitLab~~ [WIP]Support GitLab Jun 7, 2018

jayvdb added blocked and removed blocked labels Jun 7, 2018

blazeu requested changes Jun 8, 2018

View reviewed changes

bekicot force-pushed the support_gitlab branch from 8e70cec to 2284857 Compare June 9, 2018 04:21

bekicot changed the title ~~[WIP]Support GitLab~~ Support GitLab Jun 9, 2018

andrewda reviewed Jun 9, 2018

View reviewed changes

jayvdb requested changes Jun 11, 2018

View reviewed changes

jayvdb added the process/wip label Jun 11, 2018

bekicot changed the title ~~Support GitLab~~ [WIP]Support GitLab Jun 11, 2018

blazeu requested changes Jun 12, 2018

View reviewed changes

andrewda suggested changes Jun 13, 2018

View reviewed changes

bekicot force-pushed the support_gitlab branch 2 times, most recently from 3704f11 to 1b502d7 Compare July 11, 2018 10:55

blazeu requested changes Jul 11, 2018

View reviewed changes

jayvdb requested changes Jul 12, 2018

View reviewed changes

bekicot force-pushed the support_gitlab branch 2 times, most recently from 101c4b0 to ea3fc0a Compare July 12, 2018 22:06

Support GitLab Issues

6ad3ed7

Move GitHub Tasks to github namespace `/tasks/github` Add a new gitlab route `/tasks/gitlab` Add a new gitlab serializer Closes coala#38

bekicot force-pushed the support_gitlab branch from ea3fc0a to 6ad3ed7 Compare July 13, 2018 01:41

		@@ -0,0 +1,9 @@
		export const getTrackerIdentifiers = (org, trackerType) => {
		if(org && org.trackers){

		@@ -0,0 +1,50 @@
		// https://gitlab.com/api/v4/projects/coala%2Fmobans/issues?order_by=created_at&state=opened

[WIP]Support GitLab #44

Are you sure you want to change the base?

[WIP]Support GitLab #44

Conversation

bekicot commented Jun 7, 2018

jayvdb commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bekicot commented Jun 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu Jun 9, 2018 •

edited

Loading