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

Duplicate VCS content #10553

Merged
merged 1 commit into from
Nov 26, 2024
Merged

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Nov 17, 2024

Duplicate the VCS content by include reuse, avoiding copy and paste, as I suggested in #10543 (comment).

Background

Both package and project pages have content for version control system (VCS) fields.

Note

With #9701 I'd moved VCS field content to its own page, "Version Control System Fields" page. This PR doesn't change that, neither in the table of contents or how that page is rendered.

Include Reuse

I am proposing this change, to create a folder for these includes and in that put *.rst snippet content for .. include:: inclusion by other pages. I duplicate the VCS field content, as it exists now, within both package and project pages and retain the "Version Control System Fields" page.

Note

Initially, I'd moved the VCS fields page to a new "Version Control Reference" in the table of contents, outside the "Cabal Reference", but have reverted that.

Merge Order

I would like to see this merged before #10543 so that any further changes that are in addition to the duplication of content are proposed on their own.

Warning

Copy and paste duplication (as #10543 proposes) would be vulnerable to the two copies (on the package and project pages) drifting apart. That would be a maintenance burden (if we noticed) or a user let down (if we updated one copy but not the other).


  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@malteneuss
Copy link
Collaborator

The amount of files created here are quite a lot of moving parts to avoid a bit of duplication. And i still see little benefit in a dedicated VCS pag, which is the reason for #10543. But let's see what direction other maintainers prefer.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Nov 19, 2024

To quantify a "bit of duplication", this is what would be duplicated by #10543 by copy and paste or on this pull request by using RST includes (one for each section's content).

VCS kind
^^^^^^^^

Cabal supports specifying different information for various common source
control systems. This is the name of the source control system used for a
repository. The currently recognised types are:

-  ``darcs``
-  ``git``
-  ``svn``
-  ``cvs``
-  ``mercurial`` (or alias ``hg``)
-  ``bazaar`` (or alias ``bzr``)
-  ``arch``
-  ``monotone``
-  ``pijul``

The VCS kind will determine what other fields are appropriate to specify for a
particular version control system.

VCS location
^^^^^^^^^^^^

The location of the repository, usually a URL but the exact form of this field
depends on the repository type. For example:

-  for Darcs: ``http://code.haskell.org/foo/``
-  for Git: ``https://github.com/foo/bar.git``
-  for CVS: ``[email protected]:/cvs``

VCS branch
^^^^^^^^^^

Many source control systems support the notion of a branch, as a distinct
concept from having repositories in separate locations. For example CVS, SVN and
git use branches while darcs uses different locations for different branches. If
you need to specify a branch to identify a your repository then specify it in
this field.

VCS tag
^^^^^^^

A tag identifies a particular state of a source repository.  The exact form of
the tag depends on the repository type.

VCS subdirectory
^^^^^^^^^^^^^^^^

A field of this type is always optional because it defaults to empty, which
corresponds to the root directory of the repository and is the same as
specifying ``.`` explicitly.

Some projects put the sources for multiple packages inside a single VCS
repository. This field lets you specify the relative path from the root of the
repository to the top directory for the package, i.e. the directory containing
the package's ``.cabal`` file.

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 19, 2024

I agree with @malteneuss that top-level real estate is a finite resource. I also agree with him that this PR has quite a lot going on. @philderbeast I am sure you have already thought about it, but it there any way to reduce the number of files to — say — a single vcs-include.rst?

@philderbeast
Copy link
Collaborator Author

I agree with @malteneuss that top-level real estate is a finite resource.

@ffaf1, are you talking about the "Version Control System Fields" page?

@ulysses4ever
Copy link
Collaborator

I'm not overly worried about the number of inclusions per see but I do agree with others that the top-level page doesn't pull its weight. In fact, I argued exactly that on the original PR, and I was quite surprised that it ended up going in but then again: I didn't feel strong enough about it to block it.

As for de-duplication, I think the duplication overall is quite small. It also concerns things that don't change much in our experience, so I'd leave it be.

If there are more proponents of the de-duplication proposal, I have a couple of suggestions about the directory structure:

  • rename for-duplication to something more traditional like includes.
  • instead of the vcs- prefix, put the files related to VCS in a subdirectory of includes e.g. includes/vcs.

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 19, 2024

@ffaf1, are you talking about the "Version Control System Fields" page?

Yes.

@malteneuss
Copy link
Collaborator

Introducing this new includes fragments folder structure by @ulysses4ever and moving the top-level page content into sections there, seems to be a good middle ground.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Nov 20, 2024

I have a couple of suggestions about the directory structure:

  • rename for-duplication to something more traditional like includes.
  • instead of the vcs- prefix, put the files related to VCS in a subdirectory of includes e.g. includes/vcs.

@ulysses4ever, I've done this, putting those files in a vcs folder and dropping the vcs- prefix from their filenames.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Nov 20, 2024

There are some existing includes. By using the *.inc extension I'm not seeing syntax highlighting in vscode with the reStructuredText extension for references.inc. I opted not to move or rename references.inc.

$ grep -R -E '\.\. include::' ./**/*.rst
./doc/cabal-commands.rst:.. include:: references.inc
./doc/cabal-context.rst:.. include:: references.inc
./doc/cabal-interface-stability.rst:.. include:: references.inc
./doc/cabal-package-description-file.rst:.. include:: references.inc
./doc/cabal-project-description-file.rst:.. include:: references.inc
./doc/file-format-changelog.rst:.. include:: references.inc
./doc/setup-commands.rst:.. include:: references.inc

@philderbeast
Copy link
Collaborator Author

philderbeast commented Nov 21, 2024

is there any way to reduce the number of files to — say — a single vcs-include.rst?

I picked a snippet for each section to allow for all these differences.

$ diff cabal-package-description-file.rst cabal-project-description-file.rst --unified 
--- cabal-package-description-file.rst	2024-11-20 20:29:46.263245154 -0500
+++ cabal-project-description-file.rst	2024-11-20 20:30:10.032811088 -0500

-.. pkg-field:: type: VCS kind
+.. cfg-field:: type: VCS kind
 
     This field is required.
 
     .. include:: vcs/kind.rst
 
-.. pkg-field:: location: VCS location
+.. cfg-field:: location: VCS location
 
     This field is required.
 
     .. include:: vcs/location.rst
 
-.. pkg-field:: module: token
-
-    This field is required for the CVS repository type and should not be
-    used otherwise.
-
-    CVS requires a named module, as each CVS server can host multiple
-    named repositories.
-
-.. pkg-field:: branch: VCS branch
+.. cfg-field:: branch: VCS branch
 
     This field is optional.
 
     .. include:: vcs/branch.rst
 
-.. pkg-field:: tag: VCS tag
-
-    This field is required for the ``this`` repository kind.
+.. cfg-field:: tag: VCS tag
 
-    This might be used to indicate what sources to get if someone needs to fix a
-    bug in an older branch that is no longer an active head branch.
+    This field is optional.
 
     .. include:: vcs/tag.rst
 
-.. pkg-field:: subdir: VCS subdirectory
+.. cfg-field:: subdir: VCS subdirectory list
+
+    Look in one or more subdirectories of the repository for cabal files, rather
+    than the root. This field is optional.
+
+    .. include:: vcs/subdir.rst
 
-    This field is optional but, if given, specifies a single subdirectory.
+.. cfg-field:: post-checkout-command: command
 
-    .. include:: vcs/subdir.rst
\ No newline at end of file
+    Run command in the checked out repository, prior sdisting.
\ No newline at end of file

I'd tried using :start-line: and :end-line: options for .. include:: to pick line ranges to include from a single file but that was more trouble than it was worth.

@philderbeast
Copy link
Collaborator Author

I agree with @malteneuss that top-level real estate is a finite resource. I also agree with him that this PR has quite a lot going on.

Actually I'd tried to keep this PR tight, it only does the duplication using includes. One exception to that was moving the VCS page itself out of the "CABAL REFERENCE" in the table of contents. I did that in response to a comment by @geekosaur, #10543 (comment).

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

It's unfortunate that ReST doesn't support ::include-ing sections, only whole files.

Also, I'll remind you of Depends-on: in PRs to ensure things are merged in the right order.

@philderbeast
Copy link
Collaborator Author

Also, I'll remind you of Depends-on: in PRs to ensure things are merged in the right order.

Thanks @geekosaur, I didn't know about this. I don't want to impose that on someone else's PR though.

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 21, 2024

I'd tried using :start-line: and :end-line:

Was :start-after: clumsy to use too?

@philderbeast
Copy link
Collaborator Author

philderbeast commented Nov 21, 2024

I'd tried using :start-line: and :end-line:

Was :start-after: clumsy to use too?

Didn't bother with that option as by then I was convinced a simple import without options was so much simpler.

@malteneuss
Copy link
Collaborator

I can live with the many small snippets files as they are isolated well now, but i'm even less d'accord with making VCS a top-level title. Getting it out of the top-level pages doctree is my main wish here as we already have a long detailed guide page on the VCS topic, with references to the corresponding reference sections (now without duplication).

@philderbeast philderbeast force-pushed the doc/vcs-dup-via-include-9214 branch from b57aab4 to c6a99e7 Compare November 22, 2024 21:03
@philderbeast
Copy link
Collaborator Author

but i'm even less d'accord with making VCS a top-level title. Getting it out of the top-level pages doctree is my main wish here as we already have a long detailed guide page on the VCS topic, with references to the corresponding reference sections (now without duplication).

@malteneuss, I've reverted the table of contents commit.

This PR makes no change to the VCS top-level title, not in its place in the table of contents nor how it is rendered.

I'd like to keep this one about adding (duplicating) VCS content to the package and project pages. You'd like to remove the VCS top-level title. That's what you're proposing in #10543. Can we please keep removal discussions going there and not here?

@malteneuss malteneuss added the squash+merge me Tell Mergify Bot to squash-merge label Nov 23, 2024
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Nov 23, 2024
@philderbeast philderbeast force-pushed the doc/vcs-dup-via-include-9214 branch from bb3efe2 to f7523bb Compare November 23, 2024 14:02
@philderbeast philderbeast added merge+no rebase and removed squash+merge me Tell Mergify Bot to squash-merge labels Nov 23, 2024
@philderbeast
Copy link
Collaborator Author

Label merge+no rebase is necessary when the pull request is from an organisation.

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 23, 2024

Congrats on good collaboration skills you two!

- Break VCS sections into files for inclusion
- Add VCS kind content to package and project
- Add VCS location content to package and project
- Add VCS branch content to package and project
- Add VCS kind content to package and project
- Add VCS subdir content to package and project
- Put "is required" first for CVS field
- Move VCS out of the "Cabal Reference"
- Rename include files to vcs/*
- Revert "Move VCS out of the "Cabal Reference""
@philderbeast philderbeast force-pushed the doc/vcs-dup-via-include-9214 branch from f7523bb to c635ce0 Compare November 24, 2024 17:13
@philderbeast
Copy link
Collaborator Author

@mergify refresh

Copy link
Contributor

mergify bot commented Nov 25, 2024

refresh

✅ Pull request refreshed

@geekosaur
Copy link
Collaborator

I think your rebase yesterday reset the timer.

(The reason a rebase after the timer has expired doesn't affect this is that it looks for the merge delay passed label, which won't be removed by a rebase so Mergify will still consider it ready to go.)

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 26, 2024
@mergify mergify bot merged commit b62b694 into haskell:master Nov 26, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants