Skip to content
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

File support #126

Closed
wants to merge 10 commits into from
Closed

File support #126

wants to merge 10 commits into from

Conversation

quasimik
Copy link

Regarding this issue
Following this spec
A bunch of code ripped from this repo

@sreerajkksd
Copy link

@quasimik Would you have time to implement this feature in another one or two months ? I'm very much impressed with this library, but lack of file support is a bit annoying. Let me know if you want my help with anything.

@quasimik
Copy link
Author

The core functionality is already implemented. However, it's failing some lint tests, probably quite simple to fix for someone with knowledge of the library's test process. You could go ahead and fix these errors yourself, if you'd like.

As for the core functionality, I've tried to make the code okay, but there's also some chance that I didn't follow the spec properly, or missed some corner cases, in which case extra work needs to be put in. The PR will probably attract maintainer attention once it passes the aforementioned lints.

@KingDarBoja
Copy link
Contributor

No worries, the lint test are easy to fix, I will take care of those this weekend.

@KingDarBoja KingDarBoja added type: documentation An issue or pull request for improving or updating the documentation type: feature A new feature labels Sep 20, 2020
@leszekhanusz
Copy link
Collaborator

leszekhanusz commented Sep 29, 2020

This is the next issue I would like to work on (probably next weekend?)
What I don't really like as it is:

  • the files are put in the variable_values argument and we need to parse it recursively in all cases even when there is no files (99% of cases)

̶I̶ ̶w̶o̶u̶l̶d̶ ̶s̶u̶g̶g̶e̶s̶t̶ ̶t̶o̶ ̶c̶r̶e̶a̶t̶e̶ ̶a̶n̶o̶t̶h̶e̶r̶ ̶a̶r̶g̶u̶m̶e̶n̶t̶ ̶s̶p̶e̶c̶i̶f̶i̶c̶a̶l̶l̶y̶ ̶f̶o̶r̶ ̶f̶i̶l̶e̶s̶.̶
EDIT: I just added a new flag upload_files

Then we would need tests to get the code coverage to 100%

@leszekhanusz
Copy link
Collaborator

leszekhanusz commented Oct 3, 2020

I have added tests and docs but cannot push on the PR, Did you uncheck the checkbox to allow maintainers to push to this PR ?
remote: Permission to quasimik/gql.git denied to leszekhanusz.

@leszekhanusz
Copy link
Collaborator

So, because I don't have access to your repo, I created a PR for this PR.
quasimik#1

Please accept it 😁

@leszekhanusz leszekhanusz mentioned this pull request Oct 6, 2020
@leszekhanusz
Copy link
Collaborator

Thanks for your help.
PR continues in #145
Comments are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation An issue or pull request for improving or updating the documentation type: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants