Skip to content

add option -no-indent to remove indentation #1856

New issue

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

By clicking “No 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? No Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Salka1988
Copy link

@Salka1988 Salka1988 commented Jul 14, 2022

In this PR, I have added support for the new LinkType::IncludeNoIndent. This feature can be utilized with the Anchor syntax, such as {{#include file.rs:anchor_name:-no-indent}}, or with the Range syntax, {{#include file.rs::20:-no-indent}}.

Parsing is done in the same way as with the Range(LineRange) type. This means that if we use {{#include file.rs::20: 30}}, the command will not work until the whitespace between 20 and 30 is removed, resulting in 20:30. The same applies to the -no-indent command, {{#include file.rs::20: -no-indent}} will not work, while {{#include file.rs::20:-no-indent}} does.

The parsing process begins in the parse_include_path function where we check for the presence of the -no-indent command. Based on this, we return LinkType::IncludeNoIndent or only LinkType::Include.

If we have returned LinkType::IncludeNoIndent, the take_format_remove_indent function will find the part of the code closest to the left margin and move it to the left by that much space. The format of the code remains the same, with only the indentation being removed.

{{#include file.rs::20:30}}

Screenshot from 2023-01-23 14-16-07

{{#include file.rs::20:30:-no-indent}}

Screenshot from 2023-01-23 14-15-47

{{#include ../../../examples/providers/src/lib.rs:setup_test_blockchain}}

Screenshot from 2023-01-23 14-16-51

{{#include ../../../examples/providers/src/lib.rs:setup_test_blockchain:-no-indent}}

Screenshot from 2023-01-23 14-17-14

@Salka1988
Copy link
Author

Salka1988 commented Jul 17, 2022

@Salka1988
Copy link
Author

ehuss Hello, Hello :)

@Dylan-DPC
Copy link
Member

hi thanks. just give us some time we will review this :)

@Salka1988 Salka1988 force-pushed the add_option_remove_indentation branch from d67b599 to bc0f19c Compare September 25, 2022 10:57
@sanity
Copy link

sanity commented Oct 24, 2022

Hi, I'm also having this problem - did this get merged?

Is the idea to remove all indentation from code snippets or only "extra" indentation? Hopefully, the latter.

@Salka1988
Copy link
Author

Hi, I'm also having this problem - did this get merged?

Is the idea to remove all indentation from code snippets or only "extra" indentation? Hopefully, the latter.

No.

@sanity
Copy link

sanity commented Nov 3, 2022

Hi, I'm also having this problem - did this get merged?
Is the idea to remove all indentation from code snippets or only "extra" indentation? Hopefully, the latter.

No.

Do you mean that it will remove all indentation, ie.

   fn test() {
      cat.foo();
   }

Becomes:

fn test() {
cat.foo();
}

Or (what I'm hoping):

fn test() {
   cat.foo();
}

@Salka1988
Copy link
Author

Hi, I'm also having this problem - did this get merged?
Is the idea to remove all indentation from code snippets or only "extra" indentation? Hopefully, the latter.

No.

Do you mean that it will remove all indentation, ie.

   fn test() {
      cat.foo();
   }

Becomes:

fn test() {
cat.foo();
}

Or (what I'm hoping):

fn test() {
   cat.foo();
}

Second option. What you need too :)

@rachitnigam
Copy link

Just pinging this thread to say that I've also run into this problem and would find this PR really useful when merged

@sanity
Copy link

sanity commented Dec 13, 2022

Do we expect this to get merged soon? If not I may create a fork to use in the meantime, this would solve some headaches.

@Dylan-DPC
Copy link
Member

If you can test it and ensure that it works as indented intended, it will make it easier for us to review & merge :)

@sanity
Copy link

sanity commented Dec 13, 2022

🤣 I'll try to find some time.

@ehuss
Copy link
Contributor

ehuss commented Dec 13, 2022

I would prefer to avoid adding an option for this. I would expect there to be one true behavior.

There's also PRs and issues about indentation (#1564, #1565, #1718). Whatever solution is taken, I would like to see a clear description of the issue and why a particular solution was chosen. This particular PR doesn't have much of a description.

Another thing to consider is the behavior around hidden lines.

@Salka1988
Copy link
Author

@ehuss @sanity @Dylan-DPC I added some description! Can you take a look?

@ehuss
Copy link
Contributor

ehuss commented Jan 24, 2023

Can you respond to the questions raised in #1856 (comment)? Would it be possible to avoid requiring an option to determine the indentation behavior? Why choose this behavior over the other PRs?

@Salka1988
Copy link
Author

Salka1988 commented Jan 24, 2023

I think that in these PR (#1564, #1565, #1718) indent commands are not optional. What will we do if the user likes the way MdBook currently formats the text?
That's why I added a new option that is easy to use.
There is an option to expand Include(PathBuf, RangeOrAnchor, "/ indent/no_indent /").

@ehuss

@sanity
Copy link

sanity commented Jan 24, 2023

@Salka1988 - thank you for working on this :)

Opt-in makes sense from the perspective of not breaking existing code, although for me I would prefer a way to set this flag globally for all {{#include}}s rather than for each individually.

Still, this would be a big improvement over not having this functionality.

This means that if we use {{#include file.rs::20: 30}}, the command will not work until the whitespace between 20 and 30 is removed, resulting in 20:30. The same applies to the -no-indent command, {{#include file.rs::20: -no-indent}} will not work, while {{#include file.rs::20:-no-indent}} does.

I understand there may be internal reasons for it, this is definitely going to generate bug reports without a helpful error message :)

Narsil added a commit to huggingface/candle that referenced this pull request Aug 1, 2023
- Loading with memmap
- Loading a sharded tensor
- Moved some snippets to `candle-examples/src/lib.rs` This is because
managing book specific dependencies is a pain rust-lang/mdBook#706
- This causes a non aligned inclusion  rust-lang/mdBook#1856 which we have
to ignore fmt to remove.

mdbook might need some more love :)
Narsil added a commit to huggingface/candle that referenced this pull request Aug 1, 2023
- Loading with memmap
- Loading a sharded tensor
- Moved some snippets to `candle-examples/src/lib.rs` This is because
managing book specific dependencies is a pain rust-lang/mdBook#706
- This causes a non aligned inclusion  rust-lang/mdBook#1856 which we have
to ignore fmt to remove.

mdbook might need some more love :)
Narsil added a commit to huggingface/candle that referenced this pull request Aug 2, 2023
- Loading with memmap
- Loading a sharded tensor
- Moved some snippets to `candle-examples/src/lib.rs` This is because
managing book specific dependencies is a pain rust-lang/mdBook#706
- This causes a non aligned inclusion  rust-lang/mdBook#1856 which we have
to ignore fmt to remove.

mdbook might need some more love :)
Narsil added a commit to huggingface/candle that referenced this pull request Aug 2, 2023
- Loading with memmap
- Loading a sharded tensor
- Moved some snippets to `candle-examples/src/lib.rs` This is because
managing book specific dependencies is a pain rust-lang/mdBook#706
- This causes a non aligned inclusion  rust-lang/mdBook#1856 which we have
to ignore fmt to remove.

mdbook might need some more love :)
Narsil added a commit to huggingface/candle that referenced this pull request Aug 2, 2023
- Loading with memmap
- Loading a sharded tensor
- Moved some snippets to `candle-examples/src/lib.rs` This is because
managing book specific dependencies is a pain rust-lang/mdBook#706
- This causes a non aligned inclusion  rust-lang/mdBook#1856 which we have
to ignore fmt to remove.

mdbook might need some more love :)
@mattburgess
Copy link

Just to add my opinion as a user here. The behaviour described in #1564 and #1626 is clearly incorrect so shouldn't require an option on the part of the user to get correct behaviour. The PR at #1718 implements exactly the solution that #1626 describes. So, if it were up to me, (which it clearly isn't 😄 ) I'd do the following:

@julio4
Copy link

julio4 commented Dec 13, 2023

Any progress on this PR?

No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants