Updated to add pathname and made functions pure, fixed broken tests

[Akkuma]

pathname was added to the request to mimic what a URL contains, which was what req.url contained. While I was in there I updated all the functions to be pure and associated tests to use these new pure functions. I also fixed the 5 tests that were failing due to lacking the charset in the content-type.

[coveralls]

Coverage decreased (-0.02%) to 99.01% when pulling 26c388ef3d833267b714bb343cb13d5c55bb9b1f on Akkuma:master into 55a3cf755fc0d3be07857107c74da84b62a7d7e7 on spirit-js:master.

[coveralls]

Coverage decreased (-0.02%) to 99.01% when pulling 26c388ef3d833267b714bb343cb13d5c55bb9b1f on Akkuma:master into 55a3cf755fc0d3be07857107c74da84b62a7d7e7 on spirit-js:master.

[hnry]

Hey thanks for taking the time out for this, but there's some issues. It would be better if you went with the coding style I have (not that yours or mine is better, just for clarity for what's been already written). For example, no camelCase, no semi-colons.

Also it looks like a lot of changes, but half of it comes from renaming functions (I don't think we should do that here). Stick with the same names, so less unnecessary changes.

I also don't understand the changes to request, why 'pathname' micmic 'url' ? You just prefer the name pathname more?

Also added some notes in the code.

Also I have to check the charset text being added now, and see why, otherwise it'll break old node's running the same test I think.

I do like the idea of refactoring you have. Just need some clarification for the other changes and clean up.

[Akkuma]

It would be better if you went with the coding style I have (not that yours or mine is better, just for clarity for what's been already written). For example, no camelCase, no semi-colons.

I'll update this. I'm not used to your style so messed it up in some places.

Also it looks like a lot of changes, but half of it comes from renaming functions (I don't think we should do that here). Stick with the same names, so less unnecessary changes.

Got it will update that.

I also don't understand the changes to request, why 'pathname' micmic 'url' ? You just prefer the name pathname more?

pathname is what the standard url lib uses when you parse a url, which simplifies to me understanding the framework and what the url is, since I feel like the term gets heavily overloaded.

[coveralls]

Coverage decreased (-0.006%) to 99.02% when pulling fef8db4b197d0dfe30f5f9d88207e9cafa364eb0 on Akkuma:master into 55a3cf755fc0d3be07857107c74da84b62a7d7e7 on spirit-js:master.

[Akkuma]

@hnry updated it all, the diff unfortunately still looks pretty sloppy even after making the functions the same names and as expressions.

[hnry]

Hey glad to see you're still around. Looks better, by the way it pulled in package-lock.json too. I'll remove it no worries, also might do some minor edits.

I'll merge by this weekend. Thanks!

[hnry]

I started to merge and look through it, and I don't think I am willing to go with the refactor of http/request.js... I just don't want to make the changes without a real tangible benefit. Your approach is better, but I think it's best right now not to change something that isn't broken or has some immediate benefit.

I would like to pull in the changes for the failing tests (because the 'mime' package updated which broke them). But can't cherry pick just those changes. Same for pathname, it's been added.

Sorry for the trouble, and thank you.

[Akkuma]

I don't understand this train of thought. The "refactor" was all self contained with all the tests updated. This wasn't a viral refactor that impacted much of the code base. spirit isn't even labelled as a v1. If you aren't willing to make actual changes to the codebase you should note that this project is essentially frozen.

[hnry]

2/3 changes (mine package update breaking tests & pathname) were added.

I wish I could credit you more for this, but I couldn’t cherry pick the changes since you had it all grouped into a commit.

The project isn’t frozen, though I haven’t had the time to add features that I have in mind. But I’m very willing to add via contributions.

The reason is really the changes do not add a feature, fix a bug, improve performance, or anything. It’s really more vanity and readability. It has no impact except it might introduce a bug or affect performance negativitly. So that’s why I do not want to pull this specific change in.

Again I did include your other changes, so I am willing to have changes be done.

[Akkuma]

The reason is really the changes do not add a feature, fix a bug, improve performance, or anything. It’s really more vanity and readability. It has no impact except it might introduce a bug or affect performance negativitly. So that’s why I do not want to pull this specific change in.

It could improve performance by creating the same request object hidden class rather than potentially multiple hidden classes (the mutation of the request being the problem). There's also more "slots" for fast property access as all the properties are added during object creation rather than after.

It adds a feature of practicing what you preach. If the framework is designed for functional programming I expect the same from the codebase.

If you're afraid of a bug or affect performance negatively then there needs to be appropriate tests added and performance tests. Anything you add falls under the same concern then of potentially impacting performance.

Lastly you literally said Your approach is better. You should be using it if it actually is better this could be sat on for a month or however long to verify there is no performance impact and isn't something that needs to be immediately merged in.

[hnry]

There are tests, there are benchmarks. The change will most likely won't affect anything. Or as is now, it won't affect anything.

So why not just merge it? Well, when I went to review it and clean up, there were still styling issues. There were 2 broken tests still. And I do take testing very seriously, so backwards compat tests need to be written too to make sure the existing behavior is the same or otherwise noted.

For example I believe, the change will make request.query always an object, where as before it can be undefined. Needs to be confirmed, reviewed whether it will affect anything.

Is it a lot of work? No, but the issues I mentioned need to be taken care of. Which I will do. And again the other changes I would love to cherry pick it and include, but it's impossible because you grouped all the changes together.

I take testing very seriously, and just because tests will pass doesn't mean it's perfect. People can make tests pass without knowing what is really going on. For example the tests breaking because of the charset being added to the Content-Type, it's easy to make it pass, but I wanted to know why it broke (because mime package updated, didn't I lock this? I did for major, minor versions, but it changed in a patch version.).

Writing code is easy, it's the review and accountability and testing that are hard. On the surface it looks like 99% of it won't affect anything. But the 1% needs to be documented and tested to make sure before I just merge a change that isn't a feature or bug fix. If it's a feature or bug fix, then of course I will be more willing to see it merged and spend more effort too.

TL;DR If it ain't broke don't fix it (unless the 'fix' is 100% accounted for). Which it's not in this case. It's not a big deal, but the work I need to put into making this PR ready for merging, I would have done most of the work.

Other 2 changes, again, I really wish I could you credit via git history, I left a comment noting your contribution. They were fine, just couldn't cherry pick them off your repository. In the future I will ask first. But I was already looking at the source code and felt to just have it get done. If this is an issue you, you can DM me on twitter @nree your github email and I can correct this.