Repository: yshavit/mdq (rust)

Issue:
add link reference option for "no inlines"
Right now, we have three options for handling link definitions (including images):

1. Keep as-is
2. Always use inline
3. Always use full reference style: `[text][1]`

In the last option, we do at least collapsed links (`[text][]`) and shortcut links (`[text]`) slightly differently, in that rather than just assigning them a number, we have the full-reference id be the expanded text:

* `[text][]` -> `[text][text]`
* `[text]` -> `[text][text]`

This is pretty ugly and awkward. I should add an option (maybe have it be default? maybe have it be the _only_ option) to keep collapsed and shortcut links as-is, or maybe canonicalize them to either collapsed or shortcut; or maybe just auto-assign a number to them. But basically, the current approach is pretty ugly, especially if the text is a bit longish.

I should also review the various markdown specs to see how portable collapsed/shortcut links are.

Pull request:
add a LinkTransform::NeverInline option
Adds a new option, which will keep references, collapsed, and shortcut links as they were (other than reordering them -- see #390), but turns inlines into numbered full references. This new option is now the default.

Also adds a bunch of comments.

Resolves #176

## Breaking change

- Adds a new enum variant, `LinkTransform::NeverInline`. This variant is now the `LinkTransform::default()`.
- `RunOptions::default()` now uses `LinkTransform::NeverInline` as its default link formatter.

Patch:
diff --git a/src/output/link_transform.rs b/src/output/link_transform.rs
index 33a7898..b97e9ee 100644
--- a/src/output/link_transform.rs
+++ b/src/output/link_transform.rs
@@ -1,3 +1,42 @@
+/// Link transformation system for converting between different link formats.
+///
+/// # Overview
+///
+/// This module provides a flexible system for transforming links between different
+/// formats (inline, reference, etc.) while maintaining proper reference numbering and avoiding
+/// conflicts. The system is designed around a delegation pattern that separates the public
+/// API from the stateful transformation logic.
+///
+/// # Architecture
+///
+/// ## Core Components
+/// - [`LinkTransform`]: Public enum defining transformation strategies
+/// - [`LinkTransformer`]: Crate-public type for transformations; trivially delegates to [`LinkTransformerStrategy`].
+/// - [`LinkTransformerStrategy`]: Internal enum implementing the different transformation strategies
+/// - [`LinkTransformation`]: Temporary holder for preprocessing data
+/// - [`ReferenceAssigner`]: Stateful numbering system for reference links
+///
+/// ## Data Flow
+/// ```text
+/// LinkTransform (user choice)
+///       ↓
+/// LinkTransformer::from() → LinkTransformerStrategy (with ReferenceAssigner if needed)
+///       ↓
+/// For each link:
+///   LinkTransformation::new() → extract preprocessing data
+///       ↓
+///   LinkTransformation::apply() → perform transformation using strategy
+/// ```
+///
+/// The two-step process (new → apply) exists because Rust's borrowing rules prevent:
+/// ```text
+/// let result = transformer.transform(&mut self, link)
+/// //           ^^^^^^^^^^^            ^^^^
+/// //           first borrow           second borrow
+/// ```
+///
+/// By extracting data in `new()` and applying in `apply()`, we can release the borrow
+///
 use crate::md_elem::elem::*;
 use crate::md_elem::*;
 use crate::output::fmt_md_inlines::{InlineElemOptions, LinkLike, MdInlinesWriter};
@@ -22,21 +61,49 @@ pub enum LinkTransform {
     ///
     /// Links that weren't already in reference form will be auto-assigned a reference id. Links that were in reference
     /// form will have the link number be reordered.
-    #[default]
     Reference,
+
+    #[default]
+    /// Keep `[full][123]`, `[collapsed][]`, and `[shortcut]` links unchanged, but replace
+    /// `[inlined](https://example.com)` links.
+    ///
+    /// The current implementation will turn them into full-style links with incrementing numbers, but this may
+    /// change in future versions.
+    NeverInline,
 }
 
+/// The crate-public orchestrator for link transformations.
+//
+// # Internal documentation for just this file:
+//
+// This struct just provides a crate-public interface to the real logic, which is all in LinkTransformerStrategy.
 pub(crate) struct LinkTransformer {
-    delegate: LinkTransformState,
+    delegate: LinkTransformerStrategy,
 }
 
+/// Represents the display text of a link, which can be either plain text or formatted inlines.
+///
+/// This enum handles the two ways link text can be represented in the markdown AST:
+/// - Simple text strings (most common case)
+/// - Complex inline formatting like `[**bold** text](url)`
+///
+/// Used by [`ReferenceAssigner`] when it needs to convert link labels to strings for reference IDs
+/// (e.g., `[text][]` → `[text][text]`).
 #[derive(Debug, PartialEq, Eq, Hash, Clone)]
 pub(crate) enum LinkLabel<'md> {
+    /// Plain text label, potentially borrowed from the original markdown
     Text(Cow<'md, str>),
+    /// Complex label with inline formatting (emphasis, strong, etc.)
     Inline(&'md Vec<Inline>),
 }
 
 impl<'md> LinkLabel<'md> {
+    /// Converts the label to a plain string for use in reference IDs, such that they can be sorted when writing the
+    /// reference definitions.
+    ///
+    /// - For [`LinkLabel::Text`], this is straightforward string conversion.
+    /// - For [`LinkLabel::Inline`], this renders the formatted inlines back to markdown text
+    ///   (preserving formatting like `**bold**`).
     pub(crate) fn get_sort_string(&self, ctx: &'md MdContext) -> String {
         // There may be a way to Cow this so that we don't have to copy the ::Text string, but I can't find it.
         match self {
@@ -58,43 +125,58 @@ impl<'md> LinkLabel<'md> {
 impl From<LinkTransform> for LinkTransformer {
     fn from(value: LinkTransform) -> Self {
         let delegate = match value {
-            LinkTransform::Keep => LinkTransformState::Keep,
-            LinkTransform::Inline => LinkTransformState::Inline,
-            LinkTransform::Reference => LinkTransformState::Reference(ReferenceAssigner::new()),
+            LinkTransform::Keep => LinkTransformerStrategy::Keep,
+            LinkTransform::Inline => LinkTransformerStrategy::Inline,
+            LinkTransform::Reference => LinkTransformerStrategy::Reference(ReferenceAssigner::new()),
+            LinkTransform::NeverInline => LinkTransformerStrategy::NeverInline(ReferenceAssigner::new()),
         };
         Self { delegate }
     }
 }
 
-enum LinkTransformState {
+/// Strategy implementations for different link transformation approaches.
+enum LinkTransformerStrategy {
     Keep,
     Inline,
     Reference(ReferenceAssigner),
+    NeverInline(ReferenceAssigner),
 }
 
-pub(crate) struct LinkTransformation<'md> {
-    link_text: Option<Cow<'md, str>>,
-}
-
-/// A temporary holder to perform link transformations.
+/// Temporary holder for preprocessing data needed during link transformation.
 ///
-/// We need this because ownership prohibits:
+/// This struct exists to work around Rust's borrowing rules by separating the
+/// data extraction phase from the transformation phase. It holds any preprocessing
+/// data that needs to be computed before the actual transformation can occur.
 ///
+/// # Borrowing Workaround
+/// Without this pattern, we'd need to do:
 /// ```text
-/// let link_ref = self.link_transformer.transform(self, link_like)
-/// //             ^^^^^^^^^^^^^^^^^^^^^           ^^^^
-///                first borrow                    second borrow
+/// let result = transformer.transform(&mut self, link)
+/// //           ^^^^^^^^^^^            ^^^^
+/// //           first borrow           second borrow (ERROR!)
 /// ```
 ///
-/// This lets us use the `transform_variant()`'s Copy-ability to release the borrow.
+/// Instead, we can do:
+/// ```text
+/// let transformation = LinkTransformation::new(variant, writer, link); // extract data
+/// let result = transformation.apply(&mut transformer, link_ref);       // use data
+/// ```
+pub(crate) struct LinkTransformation<'md> {
+    /// Text extracted from collapsed/shortcut links for use as reference IDs.
+    /// Only populated for [`LinkTransform::Reference`] and [`LinkTransform::NeverInline`] when processing
+    /// [`LinkReference::Collapsed`] or [`LinkReference::Shortcut`].
+    link_text: Option<Cow<'md, str>>,
+}
+
 impl<'md> LinkTransformation<'md> {
+    /// Extracts preprocessing data needed for the given transformation strategy.
     pub(crate) fn new<L>(transform: LinkTransform, inline_writer: &mut MdInlinesWriter<'md>, item: L) -> Self
     where
         L: LinkLike<'md> + Copy,
     {
         let link_text = match transform {
             LinkTransform::Keep | LinkTransform::Inline => None,
-            LinkTransform::Reference => {
+            LinkTransform::Reference | LinkTransform::NeverInline => {
                 let (_, label, definition) = item.link_info();
                 match &definition.reference {
                     LinkReference::Inline | LinkReference::Full(_) => None,
@@ -111,36 +193,56 @@ impl<'md> LinkTransformation<'md> {
         Self { link_text }
     }
 
+    /// Applies the transformation using the preprocessed data and transformer state.
     // We could in principle return a Cow<'md, LinkReference>, and save some clones in the assigner.
     // To do that, fmt_md_inlines.rs would need to adjust to hold Cows instead of LinkLabels directly. For now, not
     // a high priority.
     pub(crate) fn apply(self, transformer: &mut LinkTransformer, link: &'md LinkReference) -> LinkReference {
         match &mut transformer.delegate {
-            LinkTransformState::Keep => Cow::Borrowed(link),
-            LinkTransformState::Inline => Cow::Owned(LinkReference::Inline),
-            LinkTransformState::Reference(assigner) => assigner.assign(self, link),
+            LinkTransformerStrategy::Keep => Cow::Borrowed(link),
+            LinkTransformerStrategy::Inline => Cow::Owned(LinkReference::Inline),
+            LinkTransformerStrategy::Reference(assigner) => assigner.assign(self, link),
+            LinkTransformerStrategy::NeverInline(assigner) => match &link {
+                LinkReference::Inline => assigner.assign_new(),
+                LinkReference::Full(prev) => assigner.assign_if_numeric(prev).unwrap_or(Cow::Borrowed(link)),
+                a @ LinkReference::Collapsed | a @ LinkReference::Shortcut => {
+                    let text_cow = self.link_text.unwrap();
+                    assigner
+                        .assign_if_numeric(text_cow.deref())
+                        .unwrap_or(Cow::Borrowed(*a))
+                }
+            },
         }
         .into_owned()
     }
 }
 
 impl LinkTransformer {
+    /// Returns the transformation variant this transformer was created with.
     pub(crate) fn transform_variant(&self) -> LinkTransform {
         match self.delegate {
-            LinkTransformState::Keep => LinkTransform::Keep,
-            LinkTransformState::Inline => LinkTransform::Inline,
-            LinkTransformState::Reference(_) => LinkTransform::Reference,
+            LinkTransformerStrategy::Keep => LinkTransform::Keep,
+            LinkTransformerStrategy::Inline => LinkTransform::Inline,
+            LinkTransformerStrategy::Reference(_) => LinkTransform::Reference,
+            LinkTransformerStrategy::NeverInline(_) => LinkTransform::NeverInline,
         }
     }
 }
 
+/// Manages reference numbering for link transformations that need auto-assigned IDs.
 struct ReferenceAssigner {
+    /// The next number to assign to a new reference.
+    ///
     /// Let's not worry about overflow. The minimum size for each link is 5 bytes (`[][1]`), so u64 of those is about
     ///  80 exabytes of markdown -- and it would be even bigger than that, since the digits get bigger. It's just not a
     /// case I'm too worried about right now.
     next_index: u64,
-    /// Mappings from old reference id to new. We store these as an Strings (not ints) so that we don't need to worry
-    /// about overflow.
+
+    /// Mappings from old numeric reference IDs to their new assigned numbers.
+    ///
+    /// This ensures that multiple references to the same ID get consistently
+    /// renumbered. We store these as Strings (not ints) so that we don't need to worry
+    /// about overflow when parsing very large numbers.
     reorderings: HashMap<String, u64>,
 }
 
@@ -152,6 +254,18 @@ impl ReferenceAssigner {
         }
     }
 
+    /// Assigns a reference ID for the given link using the Reference transform strategy.
+    ///
+    /// This method implements the full reference transformation logic:
+    /// - **Inline links**: Get a new auto-assigned number
+    /// - **Full references**: Renumber if numeric, otherwise keep as-is
+    /// - **Collapsed/Shortcut**: Use the link text as reference ID, renumbering if it's numeric
+    ///
+    /// # Reference ID Logic
+    /// For collapsed/shortcut links, the link text becomes the reference ID:
+    /// - `[example][]` → `[example][example]` (text is "example")
+    /// - `[123][]` → `[123][1]` (text is "123", gets renumbered)
+    /// - `[_123_][]` → `[_123_][_123_]` (text is "_123_", not purely numeric)
     fn assign<'md>(&mut self, state: LinkTransformation<'md>, link: &'md LinkReference) -> Cow<'md, LinkReference> {
         match &link {
             LinkReference::Inline => self.assign_new(),
@@ -166,6 +280,7 @@ impl ReferenceAssigner {
         }
     }
 
+    /// Attempts to renumber a reference ID if it's purely numeric.
     fn assign_if_numeric<'md>(&mut self, prev: &str) -> Option<Cow<'md, LinkReference>> {
         if prev.chars().all(|ch| ch.is_numeric()) {
             match self.reorderings.entry(String::from(prev)) {
@@ -180,6 +295,7 @@ impl ReferenceAssigner {
         }
     }
 
+    /// Assigns a new auto-incremented reference number.
     fn assign_new<'md>(&mut self) -> Cow<'md, LinkReference> {
         let idx_str = self.next_index.to_string();
         self.next_index += 1;
@@ -187,7 +303,9 @@ impl ReferenceAssigner {
     }
 }
 
-/// Turns the inlines into a String. Unlike [crate::output::fmt_plain_str::inlines_to_plain_string], this respects formatting spans
+/// Converts a vector of inline elements back to markdown string format.
+///
+/// Unlike [crate::output::fmt_plain_str::inlines_to_plain_string], this respects formatting spans
 /// like emphasis, strong, etc.
 fn inlines_to_string<'md>(inline_writer: &mut MdInlinesWriter<'md>, inlines: &'md Vec<Inline>) -> String {
     let mut string_writer = Output::without_text_wrapping(String::with_capacity(32)); // guess at capacity
@@ -221,6 +339,11 @@ mod tests {
         Of(LinkTransform::Reference, LinkReference::Full(_)),
         Of(LinkTransform::Reference, LinkReference::Inline),
         Of(LinkTransform::Reference, LinkReference::Shortcut),
+
+        Of(LinkTransform::NeverInline, LinkReference::Collapsed),
+        Of(LinkTransform::NeverInline, LinkReference::Full(_)),
+        Of(LinkTransform::NeverInline, LinkReference::Inline),
+        Of(LinkTransform::NeverInline, LinkReference::Shortcut),
     });
 
     mod keep {
@@ -409,6 +532,45 @@ mod tests {
         }
     }
 
+    mod never_inline {
+        use super::*;
+
+        #[test]
+        fn inline() {
+            // We could in principle have this return a Borrowed Cow, since the input and output are both Inline.
+            // But it's not really worth it, given that Inline is just a stateless enum variant and thus as cheap
+            // (or potentially even cheaper!) than a pointer.
+            check_never_inline(LinkReference::Inline, LinkReference::Full("1".to_string()));
+        }
+
+        #[test]
+        fn collapsed() {
+            check_never_inline(LinkReference::Collapsed, LinkReference::Collapsed);
+        }
+
+        #[test]
+        fn full() {
+            check_never_inline(
+                LinkReference::Full("5".to_string()),
+                LinkReference::Full("1".to_string()),
+            );
+        }
+
+        #[test]
+        fn shortcut() {
+            check_never_inline(LinkReference::Shortcut, LinkReference::Shortcut);
+        }
+
+        fn check_never_inline(link_ref: LinkReference, expect: LinkReference) {
+            Given {
+                transform: LinkTransform::NeverInline,
+                label: mdq_inline!("doesn't matter"),
+                orig_reference: link_ref,
+            }
+            .expect(expect);
+        }
+    }
+
     /// A smoke test basically to ensure that we increment values correctly. We won't test every transformation type,
     /// since the sibling sub-modules already do that.
     #[test]
@@ -466,6 +628,65 @@ mod tests {
         );
     }
 
+    #[test]
+    fn smoke_test_multi_with_never_inline() {
+        let mut transformer = LinkTransformer::from(LinkTransform::NeverInline);
+        let ctx = MdContext::empty();
+        let mut iw = MdInlinesWriter::new(
+            &ctx,
+            InlineElemOptions {
+                link_format: LinkTransform::Keep,
+                renumber_footnotes: false,
+            },
+        );
+
+        // Start with a collapse, just for fun. It should be unchanged.
+        let alpha = make_link("alpha", LinkReference::Collapsed);
+        assert_eq!(transform(&mut transformer, &mut iw, &alpha), LinkReference::Collapsed);
+
+        // Inline should turn into [bravo][1]
+        let bravo = make_link("bravo", LinkReference::Inline);
+        assert_eq!(
+            transform(&mut transformer, &mut iw, &bravo),
+            LinkReference::Full("1".to_string())
+        );
+
+        // [charlie][2] should be unchanged, *and* take up the "2" slot
+        let charlie = make_link("charlie", LinkReference::Full("2".to_string()));
+        assert_eq!(
+            transform(&mut transformer, &mut iw, &charlie),
+            LinkReference::Full("2".to_string())
+        );
+
+        // [charlie][1] should be reordered to [3]
+        let delta = make_link("charlie", LinkReference::Full("1".to_string()));
+        assert_eq!(
+            transform(&mut transformer, &mut iw, &delta),
+            LinkReference::Full("3".to_string())
+        );
+
+        // [4][] should stay as-is, but take up the "4" slot
+        let echo = make_link("4", LinkReference::Collapsed);
+        assert_eq!(
+            transform(&mut transformer, &mut iw, &echo),
+            LinkReference::Full("4".to_string())
+        );
+
+        // [3] should be renumbered to [3][5] (confusing!)
+        let foxtrot = make_link("3", LinkReference::Shortcut);
+        assert_eq!(
+            transform(&mut transformer, &mut iw, &foxtrot),
+            LinkReference::Full("5".to_string())
+        );
+
+        // The next inline should turn into [bravo][6]
+        let golf = make_link("delta", LinkReference::Inline);
+        assert_eq!(
+            transform(&mut transformer, &mut iw, &golf),
+            LinkReference::Full("6".to_string())
+        );
+    }
+
     fn transform<'md>(
         transformer: &mut LinkTransformer,
         iw: &mut MdInlinesWriter<'md>,
diff --git a/src/run/cli.rs b/src/run/cli.rs
index 750d638..8cc1d7f 100644
--- a/src/run/cli.rs
+++ b/src/run/cli.rs
@@ -137,7 +137,7 @@ create_options_structs! {
     clap(long, value_enum)
     pub footnote_pos: Option<ReferencePlacement>,
 
-    clap(long, short, value_enum, default_value_t=LinkTransform::Reference)
+    clap(long, short, value_enum, default_value_t)
     pub link_format: LinkTransform,
 
     clap(long, default_value_t = true, action = clap::ArgAction::Set)
@@ -170,7 +170,7 @@ impl Default for RunOptions {
         Self {
             link_pos: ReferencePlacement::Section,
             footnote_pos: None,
-            link_format: LinkTransform::Reference,
+            link_format: LinkTransform::NeverInline,
             renumber_footnotes: true,
             output: OutputFormat::Markdown,
             add_breaks: None,


Test patch:
diff --git a/tests/md_cases/links_references.toml b/tests/md_cases/links_references.toml
index eafd1bb..043740a 100644
--- a/tests/md_cases/links_references.toml
+++ b/tests/md_cases/links_references.toml
@@ -20,13 +20,59 @@ md = '''
 needed = false
 
 
-[expect."default"]
+[expect."default (never inline)"]
 cli_args = []
 output = '''
 - an [inline link][1]
 - a [standard reference link][2]
 - a [link with a non-numeric reference id][a]
 - a [link with a non-sequential reference id][3]
+- a [collapsed link][]
+- a [shortcut link]
+- a [link with a title][4]
+- a [link with a title that contains double-quotes][5]
+
+[1]: https://example.com
+[2]: https://example.com/1
+[3]: https://example.com/non-sequential
+[4]: https://example.com "my title"
+[5]: https://example.com 'my "alleged" title'
+[a]: https://example.com/a "with a title"
+[collapsed link]: https://example.com/collapsed
+[shortcut link]: https://example.com/shortcut
+'''
+
+
+[expect."never inline"]
+cli_args = ['--link-format', 'never-inline']
+output = '''
+- an [inline link][1]
+- a [standard reference link][2]
+- a [link with a non-numeric reference id][a]
+- a [link with a non-sequential reference id][3]
+- a [collapsed link][]
+- a [shortcut link]
+- a [link with a title][4]
+- a [link with a title that contains double-quotes][5]
+
+[1]: https://example.com
+[2]: https://example.com/1
+[3]: https://example.com/non-sequential
+[4]: https://example.com "my title"
+[5]: https://example.com 'my "alleged" title'
+[a]: https://example.com/a "with a title"
+[collapsed link]: https://example.com/collapsed
+[shortcut link]: https://example.com/shortcut
+'''
+
+
+[expect."full references"]
+cli_args = ["--link-format", "reference"]
+output = '''
+- an [inline link][1]
+- a [standard reference link][2]
+- a [link with a non-numeric reference id][a]
+- a [link with a non-sequential reference id][3]
 - a [collapsed link][collapsed link]
 - a [shortcut link][shortcut link]
 - a [link with a title][4]