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

doc: renamed doc files to .md, adapted build scripts #4747

Closed
wants to merge 4 commits into from
Closed

doc: renamed doc files to .md, adapted build scripts #4747

wants to merge 4 commits into from

Conversation

eljefedelrodeodeljefe
Copy link
Contributor

This is for consistency and a little less noise for the eye
as in vm.markdown (2 against 8 chars)

Discussion on: #4726 and #4733

PR-URL:
Reviewed-By:
Reviewed-By:

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2016

Is this what @rvagg was referring to in #4733 when he said:

Regarding renaming, I don't think you're going to get any support on this, it's unnecessary churn that makes the history too hard to follow. In general churn without strong justification gets a -1 around here for this reason.

@jasnell jasnell added the doc Issues and PRs related to the documentations. label Jan 18, 2016
@@ -7,7 +7,7 @@ JavaScript console mechanism provided by web browsers.

The module exports two specific components:

* A `Console` class with methods such as `console.log()`, `console.error()` and
* A `ConsoleJefe` class with methods such as `console.log()`, `console.error()` and

Choose a reason for hiding this comment

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

Typo, this should still be Console.

Choose a reason for hiding this comment

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

The rest of the changes in this file should also be removed, as this PR is supposed to be about renaming files only.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks like this was done as a branch off the other PR branch so a lot of those other changes were carried over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Give me a second to chop the PRs. Git is funny about certain things.

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

@eljefedelrodeodeljefe ... I appreciate the new PR to isolate this one change but this will need to be rebased off master and cleaned up to remove the additional changes from the other PR before it could land at all. Also, as @cjihrig and @rvagg indicate, I'm not sure if there's going to be enough consensus to get this landed. Personally I'm fine with renaming these, however.

@eljefedelrodeodeljefe
Copy link
Contributor Author

@jasnell after the comments above I figured that... will likely close this in favor for a new one in a bit. Sorry for the noise.

About consensus: I know. It just seemed logic to me, while doing the other PR. I am okay with it not landing. cc @cjihrig

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

No worries. Thank you for bearing with us @eljefedelrodeodeljefe ! The contributions are very welcome, it's just that sometimes it takes some tweaking here and there to get it just right! :-)

@rvagg
Copy link
Member

rvagg commented Jan 19, 2016

thanks @eljefedelrodeodeljefe, having a place to discuss this separately is a good idea

@eljefedelrodeodeljefe
Copy link
Contributor Author

@rvagg @jasnell that's alright for me. Just one thing: I'd like to have this closed or "paused" until #4733 is closed or merged. Because then I could just cherry-pick this commit. It's a lot easier for me of course :) I would have the same procedure for the typo fixes on #4733. When I would need to check out from master, and then cherry-pick from the PR there would be tons of merge conflicts.

Also currently investigating to rewrite history with this gist. The consequence though would be that the whole history would show the files as *.md.

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Jan 22, 2016
@jasnell
Copy link
Member

jasnell commented Jan 22, 2016

@eljefedelrodeodeljefe ... understood. I've applied the in progress label which should notify folks that it's a work in progress.

@eljefedelrodeodeljefe
Copy link
Contributor Author

Thx @jasnell. Just a heads up. I am running:

git filter-branch --tree-filter '  
if [ -f doc/api/_toc.markdown ]  
then
   mv doc/api/_toc.markdown doc/api/_toc.md
fi' --force HEAD  

This preserves the history, but changes filenames for all commits that refer to it. With node that's an operation that runs on 12k+ files. So for all the files it's probably gonna take another 20hrs on my machine. The history though looks good.

e0113773eeb4cca772366ed111558d2b08cb93ed_diff_ __users_jefe_documents_repos_node_upstream

@jasnell
Copy link
Member

jasnell commented Jan 23, 2016

that kind of forced push likely wouldn't work for master. We generally limit force pushes only to correct mistakes on landing only the most recent commits. It's hard to say what kind of impact that kind of bulk change would have.

@MylesBorins
Copy link
Contributor

@eljefedelrodeodeljefe is there a reason for changing the file names in the git history and not just a single commit?

@eljefedelrodeodeljefe
Copy link
Contributor Author

@jasnell Wouldn't the force push only apply to the PR branch and you would see problematic things in the diff? @thealphanerd this would be the consequence of the above command in order to preserve logs, since git mv would render the files git log with just one commit. You would need to git log --follow to see more though. I have seen several threads on the internets discussing this and that Linus is actively refusing to have a decent rename feature (for okay reasons).

@eljefedelrodeodeljefe
Copy link
Contributor Author

@jasnell we have talked this PR through in the doc-WG meeting. @thealphanerd pointed out that as soon has you have cleared your backlog for LTS we could have a shot at merging this. Would you give me a ping as soon as this is done?

@MylesBorins
Copy link
Contributor

Just tried landing this against staging and found the following conflicts

Unmerged paths:
  (use "git reset HEAD <file>..." to unstage)
  (use "git add <file>..." to mark resolution)

    both modified:   doc/api/tls.md
    both modified:   tools/doc/addon-verify.js
    both modified:   tools/doc/html.js

The conflicts themselves are pretty minimal. (edit) LGTM for lts (see below)

@eljefedelrodeodeljefe
Copy link
Contributor Author

I am rebasing this right now.

@eljefedelrodeodeljefe
Copy link
Contributor Author

@thealphanerd can you try again?

@eljefedelrodeodeljefe
Copy link
Contributor Author

/cc @nodejs/documentation due to active PRs and lots of movements in the docs, quick response would be super appreciated.

@MylesBorins
Copy link
Contributor

@eljefedelrodeodeljefe github is reporting there are still conflicts to resolve between this and master... have you taken a look at that?

Perhaps your master was not synced with upstream when you rebased?

@eljefedelrodeodeljefe
Copy link
Contributor Author

rebase now green, make doc seems to render fine.

@MylesBorins
Copy link
Contributor

The only conflict now is in the Makefile which is to be expected since there are multiple changes in the make file not present in v4.x

(Edit) LGTM see below

@techjeffharris
Copy link
Contributor

LGTM. I personally prefer .md to .markdown, but if CTC decides that there isn't enough substance beyond renames to warrant the changes to the git history, I'm going to have to trust their judgement.

@MylesBorins
Copy link
Contributor

I'm actually retroactively going to -0 on this... leaning towards -1

Even thought it was discussed above I didn't really think about the effect of completely wiping the history on the docs... although I'm not convinced that with LTS and stable being fairly in tune that is any less of an aesthetic choice than file name length. It feels like the data could turn out to be useful.

@eljefedelrodeodeljefe
Copy link
Contributor Author

we'd still have the rev parsing, but what reassured me was @chrisdickinson that there was lots of precedences for removing and renaming before. See 2014. I don't mind too much, would be a nice fresh start though. If CTC decided against it, then we obviously just let it go.

@rvagg
Copy link
Member

rvagg commented Feb 25, 2016

Is the Docs WG making a case for this? If so, perhaps we should hear a position from them?

The .markdown personally bothers me, I prefer .md, but it's pretty trivial and the desire for less churn easily overrides that—git blame gets very tedious trying to track down when/why something was changed when there's a lot of things moving around inside files and a change in filename adds an extra wrinkle to that process. I think we've set a fairly clear precedent in rejecting or pushing back on changes that introduce churn with little added value.

However, perhaps the Docs WG has a good case to make on why this is a good thing? I'm of the mind that handing more responsibility for docs/api to the Docs WG is a good thing in general and would prefer not to erect barriers to progress being made by people who are passionate about such things. I know others have a bit more of a mind for stricter quality-control but this is all about give and take.

@chrisdickinson
Copy link
Contributor

This seems like a case where the potential gain is relatively minimal, but the potential loss is also relatively minimal — the gain being that we get to use the more idiomatic extension, .md, on both the new and old docs (and, maybe, that new docs don't need to use .markdown.) The loss is that it necessitates an extra argument to git log to track changes, and necessitates that folks use git blame $REV — a mild, but surmountable negative. Notably, no history is lost with this transformation.

It's a tough call, since it's a subtle upside vs. a subtle downside, but I lean towards merging this & backporting — I am not aware of anyone depending on the .markdown extension, as most folks seem to be using the generated .html or .json docs. I tend to think that this is enough of a nit that it will keep coming up, too, so by fixing this now we bolster the upside by saving ourselves time in the future.

@stevemao
Copy link
Contributor

I agree with @chrisdickinson ,

The loss is that it necessitates an extra argument to git log to track changes, and necessitates that folks use git blame $REV

I believe it's just a bit hard for new git users. Most people who contribute to this repo are quite advanced. And what are the chances for new users to track the history of the .md files.

Although this is a nit PR, I believe the upside > downside.

@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 20, 2016

landed in 0800c0a

edit: worth mentioning that this was tested locally. I build the docs and made sure that the html looks exactly as it should

@eljefedelrodeodeljefe
Copy link
Contributor Author

Awesome, thx everyone. Even though it's churn here it stands for renovation, yay!

churn

@MylesBorins
Copy link
Contributor

@eljefedelrodeodeljefe how long have you been sitting on that graphic?

@eljefedelrodeodeljefe
Copy link
Contributor Author

I had to download and install Illustrator for an hour or two 😤

jasnell pushed a commit that referenced this pull request Apr 20, 2016
* doc: rename .markdown references in content
* doc: rename to .md in tools
* doc: rename to .md in CONTRIBUTING.md

PR-URL: #4747
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: techjeffharris
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
* doc: rename .markdown references in content
* doc: rename to .md in tools
* doc: rename to .md in CONTRIBUTING.md

PR-URL: #4747
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: techjeffharris
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@stevemao
Copy link
Contributor

Sorry for the late response @eljefedelrodeodeljefe , even git-mv didn't work?

This was referenced Apr 21, 2016
@eljefedelrodeodeljefe
Copy link
Contributor Author

@stevemao it is git mv. I think it's just git's or GitHubs diffing that shows it wrong.

@eljefedelrodeodeljefe eljefedelrodeodeljefe deleted the doc/rename-doc-files-to-md branch April 21, 2016 07:28
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
* doc: rename .markdown references in content
* doc: rename to .md in tools
* doc: rename to .md in CONTRIBUTING.md

PR-URL: #4747
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: techjeffharris
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
* doc: rename .markdown references in content
* doc: rename to .md in tools
* doc: rename to .md in CONTRIBUTING.md

PR-URL: nodejs#4747
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: techjeffharris
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
* doc: rename .markdown references in content
* doc: rename to .md in tools
* doc: rename to .md in CONTRIBUTING.md

PR-URL: #4747
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: techjeffharris
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@silverwind
Copy link
Contributor

@eljefedelrodeodeljefe can you open a backport PR for this against v4.x? Would make landing doc stuff on LTS easier.

@MylesBorins
Copy link
Contributor

@eljefedelrodeodeljefe ping re backport 😄

@eljefedelrodeodeljefe
Copy link
Contributor Author

Sorry, yes, just haven't had a chance to look into the procedure. Will likely do it tomorrow. Sorry for the delay.

@eljefedelrodeodeljefe
Copy link
Contributor Author

@thealphanerd #7507 cc @silverwind

MylesBorins pushed a commit that referenced this pull request Jul 5, 2016
Original commit:
0800c0a

    doc: git mv to .md
    * doc: rename .markdown references in content
    * doc: rename to .md in tools
    * doc: rename to .md in CONTRIBUTING.md

    PR-URL: #4747
    Reviewed-By: Myles Borins <myles.borins@gmail.com>
    Reviewed-By: techjeffharris
    Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
Original commit:
0800c0a

    doc: git mv to .md
    * doc: rename .markdown references in content
    * doc: rename to .md in tools
    * doc: rename to .md in CONTRIBUTING.md

    PR-URL: #4747
    Reviewed-By: Myles Borins <myles.borins@gmail.com>
    Reviewed-By: techjeffharris
    Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Original commit:
0800c0a

    doc: git mv to .md
    * doc: rename .markdown references in content
    * doc: rename to .md in tools
    * doc: rename to .md in CONTRIBUTING.md

    PR-URL: #4747
    Reviewed-By: Myles Borins <myles.borins@gmail.com>
    Reviewed-By: techjeffharris
    Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Original commit:
0800c0a

    doc: git mv to .md
    * doc: rename .markdown references in content
    * doc: rename to .md in tools
    * doc: rename to .md in CONTRIBUTING.md

    PR-URL: #4747
    Reviewed-By: Myles Borins <myles.borins@gmail.com>
    Reviewed-By: techjeffharris
    Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Original commit:
0800c0a

    doc: git mv to .md
    * doc: rename .markdown references in content
    * doc: rename to .md in tools
    * doc: rename to .md in CONTRIBUTING.md

    PR-URL: #4747
    Reviewed-By: Myles Borins <myles.borins@gmail.com>
    Reviewed-By: techjeffharris
    Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.