Skip to content
This repository has been archived by the owner on May 21, 2021. It is now read-only.

Add Filtering #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Filtering #1

wants to merge 1 commit into from

Conversation

firmanjml
Copy link

No description provided.

Copy link

@jgcmarins jgcmarins left a comment

Choose a reason for hiding this comment

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

can we have some tests?

@calmonr
Copy link
Owner

calmonr commented Nov 14, 2020

I'm sorry for the delay answering, it's been a crazy week for me.

Being honest I wasn't expecting pull requests anytime soon. It shows the true side of open source and community.

I wasn't thinking much on how filtering would be but I had a few ideas in mind, the reason is cause filtering is more related to the business than something generic. For example, GitHub is using enum and boolean for that. I've seen other implementations using contains, eq, lt, gt as well and even Lodash directives.

# GitHub GraphQL API
query { 
  viewer { 
    repositories(first: 2, isLocked: true, ownerAffiliations: OWNER) {
      edges {
        node {
          name
        }
      }
    }
  }
}

I really liked your implementation, it's simple but does the job. Thank you very much for that.

The only reason I'm not going to accept it now is the tests. I mentioned before that I wasn't expecting pull requests anytime soon. This boilerplate was something I did during a weekend and, even though I know it's super important, I didn't write tests. In short, it's not a problem with you or your code, but mine.

I'm actually doing this to facilitate both our lives. I will be writing tests to ensure that the project will continue to grow without issues.

I hope you understand the decision. I won't close your PR, I'm just going to hold it for a while and I will be back soon with the tests so we can accept it after making sure everything is working properly.

Thank you very much for your contribution @firmanjml and @jgcmarins.

@jgcmarins
Copy link

My suggestion is to keep everything inside a FilterInputType.
Something like this:

query { 
  viewer { 
    repositories(first: 2, filters: { isLocked: true, ownerAffiliations: OWNER }) {
      edges {
        node {
          name
        }
      }
    }
  }
}

@calmonr
Copy link
Owner

calmonr commented Dec 30, 2020

@firmanjml I'm sorry the delay but I'm glad to tell you that I made the necessary changes and we can now get back to it.

You can read more about what changed on: #3 #4 #5

Thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants