Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve English language, and internal and external linking #115

Merged
merged 1 commit into from
Oct 20, 2018

Conversation

adamretter
Copy link
Member

I have gone through the HTTP Client 1.0 spec from top to bottom.

  1. I have tried to make minor improvements to the English language to make the intention clearer.
  2. I have simplified some of the html code, reSpec does a lot of automation for you, that you don't need to duplicate e.g. you don't need to specify the id on an <a> when linking to a definition.
  3. I have improved the internal linking between terms and their definitions.
  4. I have added further external links and references where appropriate.

I think the spec still has a rather conversational style and uses too much wordy text to define things, where bullet points, or a more succinct style could be used.

I have not made any major sweeping changes to the text, as unfortunately I don't have the time to spend on that.

There are some technical issues which could be improved. However I don't think it is worth the effort, as it has been 8 years since the last draft of the spec; It seems unlikely that anyone would implement them (especially in light of the HTTP 2.0 module draft being available).

joewiz
joewiz previously requested changes Aug 31, 2018
Copy link
Member

@joewiz joewiz left a comment

Choose a reason for hiding this comment

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

Wow, this is such a major improvement! I just have a couple of small edits.

@adamretter
Copy link
Member Author

@joewiz thanks for the pointers, I am happy to incorporate those.

@fgeorges @ChristianGruen any comments?

@ChristianGruen
Copy link
Member

Great edits! And I’m always learning something when scanning thorough revisions like this.

I was wondering if it’s correct to say that “it/a function parses” something? In German, it’s actually pretty common to let objects do something, but sometime told me that it’s better to use a passive construction in English (“…is parsed by the function”), or something completely different.

Well, that’s just nitpicking (and won’t improve the spec itself). I’d be happy to see the PR merged.

@joewiz
Copy link
Member

joewiz commented Aug 31, 2018

I agree, "the function parses" sounds unusual here; perhaps we should say the client (as in the HTTP client) parses, and then the function returns a representation?

@fgeorges
Copy link
Member

fgeorges commented Sep 4, 2018

Thank you @adamretter! Not sure how I can make changes before merging. Do I merge then edit afterwards? (even for changes I do not want?) Or I simply ask you? Or anything else?

@adamretter adamretter force-pushed the http-client-1/hotfix/text branch from 1d1f004 to 46bac04 Compare September 5, 2018 03:20
@adamretter
Copy link
Member Author

@joewiz @ChristianGruen thanks, I have incorporated the feedback and rebased.

@adamretter
Copy link
Member Author

@fgeorges Good question!

So if there are problems with changes in the PR, then I would suggest to do a review. I can then fix those changes.

Otherwise, I generally view PRs like this:

  • Does the PR improve the overall situation, even if it doesn't fix everything?

If so, then I usually merge. You can then send a follow-up PR with changes, which is good as it gives others like @joewiz @ChristianGruen an opportunity to help you can any new language issues. The PR approach also keeps everyone up-to-date on what is happening. i.e. changes don't appear that no one apart from the originator knew about that.

That is to say PR's represent a community method of development.
All other Open Source projects which I contribute to, pretty much have the role that no-one, not even the project owner, directly commits to the repository. Everyone must contribute through PR's so that there is a many eyes approach.

@adamretter
Copy link
Member Author

@fgeorges any progress on this please?

@fgeorges
Copy link
Member

@adamretter I guess reviews is the way to go here then, as some changes must be discarded (so it would be weird to accept them then create another PR to reverse them...)

@fgeorges
Copy link
Member

@adamretter BTW, I can't see @joewiz 's own comments. Is it because you integrated them?

Copy link
Member

@fgeorges fgeorges left a comment

Choose a reason for hiding this comment

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

@adamretter I added some reviews (mostly details actually.) Great PR, thanks!

@adamretter adamretter force-pushed the http-client-1/hotfix/text branch from 46bac04 to 657238a Compare October 10, 2018 10:45
@adamretter
Copy link
Member Author

@fgeorges I incorporated your review. Thanks :-) Should be good to go now.

@adamretter
Copy link
Member Author

@fgeorges Have you seen the changes I made?

@fgeorges fgeorges dismissed joewiz’s stale review October 20, 2018 14:56

I believe that was incorporated in Adam's changes.

@fgeorges fgeorges merged commit 6268579 into expath:master Oct 20, 2018
@fgeorges
Copy link
Member

@adamretter Sorry, I didn't received a notif for your responses. Merged now. Thanks!

@ChristianGruen
Copy link
Member

Great!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants