Use GraphQL for GitHub #33

bekicot · 2018-05-30T03:46:40Z

Use graphql for GitHub API
Add support to coala & 52North
Add GitHub token modal
Rename implementation of Issues to Tasks
Use Bulma for task list

Regression:
Remove Repository and User Model

blazeu

There should be an issue, and the PR should close the issue.

The frontend should be left unchanged or the change should be minimal. But, instead it's being rewritten with another css library, it's certainly not in the milestone and not related to GraphQL whatsoever. But I wanna hear what the other mentors say in this.

Are you using yarn or npm, please keep it consistent, we can't have 2 lockfiles, it's cumbersome to sync them both.

There's many files with missing newline on EOF.

blazeu · 2018-05-30T03:49:03Z

app/controllers/application.js

@@ -2,15 +2,10 @@ import Controller from '@ember/controller';

 export default Controller.extend({
  toggleSidenav: true,
+


Let's keep the diff small

blazeu · 2018-05-30T03:58:32Z

app/graphql/github.js

+const TEMPLATES = {
+  tasks:
+`
+{


I'd prefer the GraphQL to be in a separate file like in gci-leaders and gh-board.

^Probably not very simple to do because of the build system design

Yup, something to do with ember-cli hook. It is possible, but not worth the pain. At least for now.

Also should be noted if we want to add more queries. (There is a shift in GitLab to also use graphql)

blazeu · 2018-05-30T04:01:27Z

app/graphql/github.js

+  rateLimit {
+    remaining
+  }
+  search(query: "{{query}}", type: ISSUE, first: 100 ) {


GraphQL has its own implementation of doing variable if you use a client for it

https://github.com/prismagraphql/graphql-request
https://github.com/alphasights/ember-graphql-adapter

If you use the Ember.js one, you'll have support for models, I think

I try to keep the dependency as simple as possible.

Probably will use graphql client, when there is a feature that we needed.

You have to.

GraphQL variable is special, it's sent separately from the query and has type checking.

You could implement the GraphQL client yourself, but it's better to use battle-tested library, just find the smallest one out there.

https://medium.com/the-graphqlhub/graphql-tour-variables-58c6abd10f56

blazeu · 2018-05-30T04:05:22Z

app/organizations.js

@@ -1,33 +0,0 @@
-export default [
-  {
-    name: 'Discourse',


It should be possible to keep the current organization.

The implementation of organization will be replaced in the task
https://gitlab.com/coala/GSoC/GSoC-2018/issues/312
using
https://gitlab.com/coala/GSoC/GSoC-2018/issues/350

that is for adding more.
Add these ones as part of this PR.
Otherwise this PR is a regression.

blazeu · 2018-05-30T04:11:54Z

app/routes/tasks.js

@@ -0,0 +1,40 @@
+import { computed } from '@ember/object';
+import Route from '@ember/routing/route';
+import {inject} from '@ember/service';


Inconsistent bracket spacing

blazeu · 2018-05-30T04:12:02Z

app/routes/tasks.js

+
+  queryParams: {
+      org: {
+         refreshModel: true


Weird indent.

blazeu · 2018-05-30T04:12:49Z

app/services/github.js

@@ -0,0 +1,46 @@
+import {inject} from '@ember/service';
+import AjaxService from 'ember-ajax/services/ajax';
+import {generate} from '../graphql/github';


Inconsistent bracket spacing

blazeu · 2018-05-30T04:16:35Z

app/templates/application.hbs

+  <div id="navbarExampleTransparentExample" class="navbar-menu">
+    <div class="navbar-end">
+      <div class="navbar-item">
+        <a href="#" onclick={{action "showSettingsModal"}} >{{fa-icon 'cog' size="2x"}}</a>


Useless space before >

blazeu · 2018-05-30T04:38:03Z

app/models/issue.js

-import DS from 'ember-data';
-import { computed } from '@ember/object';
-
-export default DS.Model.extend({


Can we keep the model? Seems like this is a feature that we have to use in Ember.js.

No need ATM, probably will re-add it when I add support for GitLab.
However, learning from the past, I will try to keep dependency as simple as possible. Adding ember data was adding uneeded complexity, for me.

The issue model was originally used for GitHub using v3 Api.

Please keep the models. We will need this level of sophistication.
Supporting multiple hosters means the app must declare its own data structure which supports a subset of each hosters features.

blazeu · 2018-05-30T04:39:30Z

Can you do #32 first so it's easier for everyone to review?

blazeu · 2018-05-30T09:12:40Z

app/services/github.js

+      })
+    }
+    const query = generate('tasks', context);
+    const request = this.request({query: query})


If you don't want to use GraphQL library, atleast send the variable, there's no need to parse or do variables replacing yourself, the server will handle it.

IIRC, variables is sent as variables key as JSON.

It doesn't parse. It transform to a github compatible search query. I don't know if github search query support array :). The query argument only support string https://developer.github.com/v4/query/#connections

@blazeu We still need to transform, even though we use library.

However, I do agree that I should not interpolate (Replacing) the query. and utilize variable instead.

bekicot · 2018-05-30T23:26:19Z

@blazeu
Will fix graphQl.
And remove package-lock

The previous implementation of the GUI, It was poorly designed and highly coupled with EmberData. Was make sense as previously I only design it to work on github.

It was my first ember project so I added stuff that I think it is cool. Now it is not extensible, extending from there was a pain, so I decide to reuse what is usable and remove the complexity. I do not rewrite, just remove some gui element that is complex, or rename it.

The idea (like issue) is still there, I rename it to task,Lbecause this project will also support pull requests.

Use Bulma because the ember-paper is too complex and hard to extend.

blazeu · 2018-05-31T04:52:58Z

I mean issue, there should be an issue here https://github.com/coala/git-task-list/issues for whatever is being done here, and the commit should close it. Edit: #34 was made after the PR.

GitLab milestone is only for keeping track of GSoC progress.

blazeu · 2018-05-31T04:57:18Z

For the frontend change, the commit doesn't explain enough that the frontend was rewritten/changed with Bulma.

Also, the commit tag graphql: is redundant, Use GraphQL for GitHub + Closes link is enough, note the capitalization though.

jayvdb · 2018-06-01T02:19:37Z

app/data/organizations.js

+    trackers: [
+      {
+        type: 'github',
+        identifier: 'coala'


Trailing , ?

jayvdb · 2018-06-01T02:22:19Z

app/organizations.js

@@ -1,33 +0,0 @@
-export default [
-  {
-    name: 'Discourse',


that is for adding more.
Add these ones as part of this PR.
Otherwise this PR is a regression.

jayvdb · 2018-06-01T02:26:13Z

app/models/issue.js

-import DS from 'ember-data';
-import { computed } from '@ember/object';
-
-export default DS.Model.extend({


Please keep the models. We will need this level of sophistication.
Supporting multiple hosters means the app must declare its own data structure which supports a subset of each hosters features.

jayvdb · 2018-06-01T02:35:01Z

It was requested at #33 (comment) that you unbreak the repo before rewriting it. Please do that.

jayvdb · 2018-06-01T03:18:59Z

This is blocked. That means you stop pushing changes here for review.
Re-read my comment above.

li-boxuan · 2018-06-01T03:10:25Z

app/graphql/github.js

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


add a space after if

li-boxuan · 2018-06-01T03:15:58Z

app/graphql/github.js

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


Better keep the use of quotes consistent. I've seen both double quotes and single quotes at many places.

blazeu · 2018-06-01T03:57:44Z

app/data/organizations.js

+        identifier: '52North'
+      }
+    ]
+  }


Move all the organizations from app/organizations.js to here first, then you can add more in another PR

It cant, the other fetches from wikidata.

blazeu

The model file is kept, and yet it isn't being used, that is beyond weird.

Modifying the model is ok to support the new data structure from GitHub.

In the upcoming task of supporting GitLab, it should also use the same model, so the app works seamlessly given the same model regardless of where the data comes.

blazeu · 2018-06-01T05:50:54Z

app/components/settings-modal.js

+import {oneWay} from '@ember/object/computed';
+import {inject} from '@ember/service';
+
+


Should be 1 blank line

jayvdb · 2018-06-13T03:25:58Z

config/coverage.js

+'use strict';
+
+module.exports = {
+  coverageEnvVar: 'COV',


this a separate commit, with a descriptive commit message.

Removed, was going to test to disable some rule.

jayvdb · 2018-06-13T03:34:31Z

Note that codecov is not showing for the PR. https://codecov.io/gh/coala/git-task-list/pull/33

Please enable codecov on your fork as a temporary solution https://codecov.io/gh/bekicot/git-task-list

jayvdb · 2018-06-13T08:49:52Z

OMG, https://travis-ci.org/bekicot/git-task-list is still not enabled, and https://travis-ci.org/bekicot doesnt show any related CI enabled.

Yana, you must test your work on your own CI on your own fork, before pushing it to your PR. That is how you avoid build errors on your PR like https://travis-ci.org/coala/git-task-list/jobs/391590378 .

jayvdb · 2018-06-13T09:01:23Z

ember-cli-build.js

@@ -2,9 +2,6 @@

 const EmberApp = require('ember-cli/lib/broccoli/ember-app');

-const environment = process.env.EMBER_ENV;


these were added in one commit, and removed in this commit.

that isnt suitable for pushing to a PR for mentors to have to find and complain about.

jayvdb · 2018-06-13T09:07:46Z

app/services/organizations.js

@@ -8,7 +8,7 @@ export default Service.extend({
    this.organizations = Object.create(organizations);
  },

-  list: computed('organizations', function () {
+  list: computed('organizations', function listOrganization() {


this is a file you added in another commit. the lint fixup should be in the original commit where you add the file.

Run the linter on your 'real' commit and fix errors there.

The lint fixup commit should then only include additional style changes to lines of code which you did not modify in your 'real' commit.

jayvdb · 2018-06-13T09:18:50Z

This PR will soon be too long to be usable on GitHub.
When that happens, please create a new branch, and a new PR, and then we close this one.

That should also fix the codecov problems.

bekicot · 2018-06-13T11:16:28Z

Ok, Will remove codecov & lint from this pull and move it to a new pull request.

jayvdb · 2018-06-13T12:05:52Z

Ok, Will remove codecov & lint from this pull and move it to a new pull request.

That is not what I requested.
Please activate https://travis-ci.org/bekicot/git-task-list and https://codecov.io/gh/bekicot/git-task-list

bekicot · 2018-06-13T12:07:47Z

Ok

bekicot · 2018-06-15T08:12:46Z

https://codecov.io/gh/bekicot/git-task-list/commit/844472bdafcda833de318c0a7345c5dcebd8b37e

Disable usupported rules for specific ember apps. Add a eslint-config-airbnb-base, eslint-plugin-ember, eslint-plugin-import to package.json. Update ember-cli-code-coverage to v1.0.0-beta.4 version to get the fix when using the `useBabelInstrumenter` config see ember-cli-code-coverage/ember-cli-code-coverage#133 . Closes coala#58

blazeu · 2018-06-27T15:45:22Z

app/styles/app.scss

-.search-results {
-  overflow-y: scroll;
+.tag {
+  text-shadow: 1px 1px 1px white;


The text shadow makes it harder to read.

Oh but it's also even harder when there's no text-shadow.

Maybe just use label color as border-color for now? If it's too hard to implement dynamic text color.

Edit: There's simple way to calculate optimal text color if you just google it.

Browser support is vary, the proper implementation is still in a draft proposal, the support is small. The other solution involves some javascript.
Maybe, i'm missing something, can you specify the link?

Yes, use Javascript.

Use GraphQL for GitHub API Add support to coala & 52North Add GitHub token modal Rename implementation of Issues to Tasks Use Bulma for task list Regression: Remove Repository and User Model Remove Search Bar Closes coala#34 Bug Fix and improvement

blazeu · 2018-06-29T15:17:40Z

Commit 1236cad

Closes should be on the last line

blazeu · 2018-06-29T15:22:28Z

app/templates/components/task-item.hbs

+        <img src="{{task.repository.owner.avatarUrl}}" alt="">
+      </figure>
+      &nbsp;
+      {{task.repository.nameWithOwner}}


I think he meant the coala part shouldn't be shown because it'll always be coala in current list.

bekicot · 2018-06-30T10:38:29Z

Continued in #64

bekicot force-pushed the use-graphql branch from d4516dd to 7b9ba81 Compare May 30, 2018 04:14

blazeu requested changes May 30, 2018

View reviewed changes

bekicot force-pushed the use-graphql branch from 7b9ba81 to 00706ba Compare May 30, 2018 05:28

blazeu reviewed May 30, 2018

View reviewed changes

bekicot force-pushed the use-graphql branch 3 times, most recently from f7a31e0 to 95fdea6 Compare May 31, 2018 04:35

bekicot force-pushed the use-graphql branch 2 times, most recently from 028be11 to da3907b Compare May 31, 2018 22:55

jayvdb requested review from hemangsk, li-boxuan, andrewda and guyzmo June 1, 2018 02:12

jayvdb reviewed Jun 1, 2018

View reviewed changes

app/data/organizations.js Outdated

trackers: [

{

type: 'github',

identifier: 'coala'

Copy link

Member

jayvdb Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing , ?

jayvdb requested changes Jun 1, 2018

View reviewed changes

jayvdb added the blocked label Jun 1, 2018

bekicot force-pushed the use-graphql branch from da3907b to 55589c3 Compare June 1, 2018 03:12

bekicot mentioned this pull request Jun 1, 2018

Add GitHub org #37

Open

li-boxuan reviewed Jun 1, 2018

View reviewed changes

blazeu requested changes Jun 1, 2018

View reviewed changes

blazeu removed the blocked label Jun 1, 2018

bekicot force-pushed the use-graphql branch 2 times, most recently from dda2e8f to fb5ba6c Compare June 13, 2018 01:38

jayvdb reviewed Jun 13, 2018

View reviewed changes

bekicot force-pushed the use-graphql branch from fb5ba6c to fb2f903 Compare June 13, 2018 05:23

jayvdb removed the request for review from guyzmo June 13, 2018 08:30

jayvdb changed the title ~~[WIP] graphql: Use graphql for GitHub~~ [WIP] Use GraphQL for GitHub Jun 13, 2018

jayvdb requested changes Jun 13, 2018

View reviewed changes

bekicot force-pushed the use-graphql branch from fb2f903 to 844472b Compare June 15, 2018 08:05

bekicot force-pushed the use-graphql branch from 844472b to 60da329 Compare June 27, 2018 05:31

blazeu requested review from jayvdb and andrewda June 27, 2018 15:35

blazeu reviewed Jun 27, 2018

View reviewed changes

bekicot added 3 commits June 28, 2018 18:09

Use GraphQL for GitHub

1236cad

Use GraphQL for GitHub API Add support to coala & 52North Add GitHub token modal Rename implementation of Issues to Tasks Use Bulma for task list Regression: Remove Repository and User Model Remove Search Bar Closes coala#34 Bug Fix and improvement

Fix lint error

7a7826d

Sync gitignore moban

7e55706

bekicot force-pushed the use-graphql branch from f6d14ed to 7e55706 Compare June 28, 2018 11:09

blazeu requested changes Jun 29, 2018

View reviewed changes

bekicot changed the title ~~[WIP] Use GraphQL for GitHub~~ Use GraphQL for GitHub Jun 30, 2018

bekicot closed this Jun 30, 2018

bekicot mentioned this pull request Jun 30, 2018

Use GraphQL for GitHub - 2 #64

Merged

		@@ -2,15 +2,10 @@ import Controller from '@ember/controller';

		export default Controller.extend({
		toggleSidenav: true,

		import {oneWay} from '@ember/object/computed';
		import {inject} from '@ember/service';

		@@ -2,9 +2,6 @@

		const EmberApp = require('ember-cli/lib/broccoli/ember-app');

		const environment = process.env.EMBER_ENV;

Use GraphQL for GitHub #33

Use GraphQL for GitHub #33

Conversation

bekicot commented May 30, 2018 • edited Loading

blazeu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu May 30, 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

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 commented May 30, 2018

blazeu May 30, 2018 • edited Loading

Choose a reason for hiding this comment

bekicot May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bekicot May 31, 2018 • edited Loading

Choose a reason for hiding this comment

bekicot commented May 30, 2018 • edited Loading

blazeu commented May 31, 2018 • edited Loading

blazeu commented May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented Jun 1, 2018

jayvdb commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu 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

jayvdb commented Jun 13, 2018 • edited Loading

jayvdb commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented Jun 13, 2018

bekicot commented Jun 13, 2018 • edited Loading

jayvdb commented Jun 13, 2018

bekicot commented Jun 13, 2018

bekicot commented Jun 15, 2018

Choose a reason for hiding this comment

blazeu Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazeu commented Jun 29, 2018

Choose a reason for hiding this comment

bekicot commented Jun 30, 2018

bekicot commented May 30, 2018 •

edited

Loading

blazeu left a comment •

edited

Loading

blazeu May 30, 2018 •

edited

Loading

blazeu May 30, 2018 •

edited

Loading

bekicot May 31, 2018 •

edited

Loading

bekicot May 31, 2018 •

edited

Loading

bekicot commented May 30, 2018 •

edited

Loading

blazeu commented May 31, 2018 •

edited

Loading

blazeu commented May 31, 2018 •

edited

Loading

blazeu Jun 1, 2018 •

edited

Loading

jayvdb commented Jun 13, 2018 •

edited

Loading

bekicot commented Jun 13, 2018 •

edited

Loading

blazeu Jun 27, 2018 •

edited

Loading