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

Add HTTP::FormData parsing and generating #3243

Closed
wants to merge 6 commits into from

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Sep 3, 2016

Depends on #2967

@RX14
Copy link
Contributor Author

RX14 commented Sep 7, 2016

@asterite I have implemented your suggestions and implemented multipart parsing as a pull parser. Can we please get this merged soon, as @sdogruyol and I have been waiting on this to get merged for a long time just for it to get rejected "at the last second" after expecting it to make it into 0.19.0.

Seriously, this issue is 4 days old and has no tags, and no response from the core team, and this PR isn't the only one. I'm not impressed. I know #3234's author isn't impressed:

 <+meew0> also why is nobody commenting on my issue
 <+meew0> I literally gave them the fix

@RX14
Copy link
Contributor Author

RX14 commented Sep 7, 2016

To put a more positive spin on this issue: can we have a standard of every issue/PR being tagged and a response from a repo contributor within 24h? Even if that response is just a "This is quite a big PR, i'll have to take some time to review it".

I'm actually surprised how inconsistent it is, some issues get solved and patched within 12h, and some "fall through the cracks" and get no replies at all. Maybe there should be a bot that pings core contributors if there has been no reply to a new issue in 24h?

@asterite
Copy link
Member

asterite commented Sep 7, 2016

Is there any reason why this can't be provided as a shard?

@RX14
Copy link
Contributor Author

RX14 commented Sep 7, 2016

@asterite See: #1989 (comment)

@sdogruyol
Copy link
Member

sdogruyol commented Sep 7, 2016

I'm just curious about @asterite's opinion here. You don't want to have multipart in std?

@asterite
Copy link
Member

asterite commented Sep 7, 2016

Yes, I want to have multipart in the standard library. I also wanted to have non-blocking signal handling, and other concurrent features, so we accepted some contributions without reviewing them properly, with time, and checking if they matched our ways of shaping the std. And now we have issues with signals, broken pipes, etc, which puts more burden and work on the project. I'd like to avoid that.

This is why I suggest to put this work in a shard. The shard can evolve independently of my time and will to review these things. Once it's mature enough and it has passed the test of time we can consider putting it in the standard library.

@RX14
Copy link
Contributor Author

RX14 commented Sep 7, 2016

@asterite Is this a new policy because my previous PRs, specifically #2973 (which is arguably a more complex piece of code) have been reviewed and merged pretty promptly. AFAIK #2967 is pretty much unchanged from when @jhass reviewed it when I initially wrote the code, over 2 months ago.

I would also appreciate a response to the other, wider issues I raised in my comment. A commitment to keep this issue tracker responsive and welcoming by triaging issues and PRs withing 24h would be most welcome.

@asterite
Copy link
Member

asterite commented Sep 8, 2016

@RX14 the policy has always been the same: whenever we have time, and whenever we manage to tackle issues depending on the complexity (complexity is subjective, of course). In the case of IO::Delimited it's just a single class with one specific purpose, and I took the time to review it and understand it. Multipart is a tad more complex and requires an API design (IO::Delimited just follows the IO API).

I also can imagine multipart is needed for receiving file uploads in a server, but our HTTP::Server doesn't support an IO in the request, so I can't see how would adding a multipart class will improve this situation.

Also note that this is not an emergency hospital and nobody is paying us support, so expecting someone to answer within 24hs isn't a serious expectation at this point.

@sdogruyol
Copy link
Member

sdogruyol commented Sep 8, 2016

@asterite i really believe that the support (both financial and community-wise) will grow stronger as the interest for Crystal is really peaking up 💯

@RX14
Copy link
Contributor Author

RX14 commented Sep 9, 2016

The policy has always been the same: whenever we have time, and whenever we manage to tackle issues depending on the complexity (complexity is subjective, of course). In the case of IO::Delimited it's just a single class with one specific purpose, and I took the time to review it and understand it. Multipart is a tad more complex and requires an API design (IO::Delimited just follows the IO API).

This makes sense. API design is a somewhat complex topic.

I also can imagine multipart is needed for receiving file uploads in a server, but our HTTP::Server doesn't support an IO in the request, so I can't see how would adding a multipart class will improve this situation.

Yeah, I think a portion of HTTP needs a rewrite to use IO properly. I started this but never got very far, unfortunately.

Also note that this is not an emergency hospital and nobody is paying us support, so expecting someone to answer within 24hs isn't a serious expectation at this point.

Except it makes the crystal issue tracker feel unwelcoming, which I feel drives newbies away. Imagine working on your first program in crystal, and hitting a bug in the standard library, you assume the crystal stdlib probably doesn't have errors so go checking your own code first. After a while you conclude that the stdlib is bugged. You've could have spent several hours on it at this point, so you write up an issue on the crystal bugtracker... and it "falls through the cracks" and gets ignored. You can see that the issue tracker is still active on other issue but are confused and frustrated as to why your issue isn't getting traction, or response at all. Have you done something wrong? Is it your fault? This doesn't feel like a welcoming situation to me.

I'm not asking for issues to be solved within 24h (although working up a backlog of small bugs is bad IMHO), I'm simply asking for them to be tagged and a preliminary welcoming response if it's a new/unknown contributor. Make them feel welcome and like people care about their issue - and the language. Tagging the issue and a few words of welcoming or reassurance that their issue is being looked at takes 5 minutes at the most.

Over the past month, the crystal bugtracker has has 9 new pull requests, and 42 new issues. That's about 1.7 issues/PRs per day on average. Doing this isn't even adding 10 minutes to your day, and creates a much more welcoming atmosphere to new contributors. I think the benefits are clear to me.

@RX14
Copy link
Contributor Author

RX14 commented Sep 9, 2016

Actually, the statistics I got from github pulse only include issues and PRs that haven't been closed. The real number is about 167 issues/prs per month. That sounds like a more believable number.

There are 57 new issues this month that remain untagged (34%). There are 19 new issues which have no comments (11%). That's tagging 2 extra issues per day and commenting on two extra issues every 3 days. I don't think that's too much of an extra burden.

@mverzilli
Copy link

@RX14 you make valuable points in your last 3 paragraphs and thank you for making them in a nice, non-aggressive manner. Contrary to what many people think, that makes a huge difference.

I think you're right in that having some basic "quick issue feedback protocol" would be helpful to make newcomers feel more welcome.

I also think that putting that pressure on Ary alone is a bit unfair. For example this:

Over the past month, the crystal bugtracker has has 9 new pull requests, and 42 new issues. That's about 1.7 issues/PRs per day on average. Doing this isn't even adding 10 minutes to your day, and creates a much more welcoming atmosphere to new contributors. I think the benefits are clear to me.

The "10 minutes per day" argument is tricky. It doesn't sound like a big deal when you put it that way.

But there are 1440 minutes in a day.
480 minutes for a good night's sleep.
120 minutes for having reasonably quiet meals.
30 minutes for exercising.
Say 240 minutes for working on non-Crystal stuff.

That leaves him with 570 minutes for EVERYTHING ELSE.

That includes in Crystalworld: thinking of generics, parallelism, explaining for the 100th time why there's no Windows support (yet), fixing bugs of the current release so it feels less buggy, deciding what to include in the next release, ensuring documentation is up to date, showing up from time to time in the IRC, responding to questions in the Google Group, and dealing with amazingly high quantities of entitled pseudo-contributors who do nothing else than complaining, threatening with forks and deride his work if he happens to disagree with them.

A fair share of those 570 also needs to go to life outside of Crystalworld: quality time for loved ones, leisure, heck, investing some time in actually doing nothing is really important for your mental sanity.

As in everything, there are two sides to this: core contributors and maybe some of us at Manas could try to figure out a way of responding faster without that meaning more work for Ary, but less.

The other side is for folks that come to the project to understand that the folks running the project are also people, their time is a finite resource, and that actually yes, those 10 additional minutes may be too much of an extra burden.

@RX14
Copy link
Contributor Author

RX14 commented Sep 9, 2016

@mverzilli I certainly don't want to put extra pressure on asterite unnecessarily. Asterite does loads of amazing work on crystal, and I don't want to take away from Ary's crystal's development time - or other time - unnecessarily. This issue is as usual not black and white.

Perhaps certain trusted community members could be allowed to tag issues and otherwise manage the issue tracker, if it becomes too much of a time hole for asterite. I'm not sure if github lets you add people to the issue tracker without letting them commit to master though.

@sdogruyol
Copy link
Member

I agree with @RX14 about issue tracking. Certain capable and trusted community members can do this kind of tagging, reducing e.g. After some time this could save lots of time.

@asterite
Copy link
Member

First, let me agree with @mverzilli and repeat that I appreciate this discussion to be civilized and non-aggressive. Thank you! And thank you for everything you do for Crystal (PR, suggestions, replying on IRC and Google Groups).

@RX14 What kind of reply do you think would be helpful? I can imagine something like "Thank you for opening this issue / sending this PR, we'll take a look at it soon", although "soon" is probably a lie and depends on many factors. Since I wouldn't like to lie, what can we reply?

Another issue I have with replying things is that it's not just one or two minutes. One reply often leads to another reply, many times a complaint, which makes me want to reply again and explain the situation.

Let's take an example. #3262 doesn't have an answer from me or from any core member. My opinion is that XML is not needed in every program, so that's why we don't require libxml2, or libyaml, or libgmp, in distributions. But if I answer this I know what will happen, based on my previous experience: someone will disagree, and suggest that we should require that even if it's not always needed. Or maybe we should implement our own XML/YAML/BigInt things. If I leave that without an answer it's almost the same as if I didn't answer the first question.

Or this issue itself: I don't want to merge things that I didn't fully review, with at least some other core member, like @waj. In the past we've merged some things related to concurrency, for example, and now we have bugs and we can't easily understand them because we didn't write that code. Not only that, but we received many (harsh) complaints later, because we merged some code from that person, but not all code.

We prefer to exclude some features and take the time to implement them ourselves, with good docs and clear code, then accepting something that we'll later regret. Not that I'm saying your code is bad or badly documented, it's just that we need time to review it. I already suggested to move it to a shard, but that had a reply and the issue remains open. What else can I do? Should I blindly merge this?

@asterite
Copy link
Member

@omarroth
Copy link
Contributor

@RX14 The closest to issue-only permissions I could find was this, but it's a bit of a hack and doesn't seem to provide a way of properly tagging issues.

On another note, there does already appear to be a http parser for Crystal, however it's a wrapper for a C library, not an implementation in Crystal. @sdogruyol Maybe you would like to use that for now?

@RX14
Copy link
Contributor Author

RX14 commented Sep 11, 2016

@omarroth That option has already been discussed. It's not feasible because you need an external shared library. The optimal solution would either be to get this merged or create a shard, and it looks like i'll have to do the latter.

My fear is that if this becomes a shard, the likeliness of it ever being merged into the stdlib decreases because the pressure from the people using it to get it merged decreases which means it goes on the backburner and there's always something more important than merging this PR.

@sdogruyol
Copy link
Member

Up up 👍

@ozra
Copy link
Contributor

ozra commented Sep 14, 2016

Having certain modules as shards first, and then consider them for implementation into stdlib, after they've stabilized and shown to not require hideous amounts of maintenance, sounds like a good thing to me.

Changes get out quicker also, instead of having to wait on a new compiler release. I think it will be good for the module!

@sdogruyol
Copy link
Member

I'm really sad about this 😢 Can't we at least get a shard @RX14 ?

@RX14 RX14 mentioned this pull request Oct 1, 2016
@RX14 RX14 closed this Dec 18, 2016
@RX14
Copy link
Contributor Author

RX14 commented Dec 18, 2016

See #2967 for resolution.

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

Successfully merging this pull request may close these issues.

None yet

6 participants