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

[format] Make the code respect the documentation for set_max_indent #1525

Merged
merged 2 commits into from Jan 16, 2019

Conversation

Projects
None yet
4 participants
@rbonichon
Copy link
Contributor

commented Dec 11, 2017

Nothing should happen when new maximum indentation is smaller than 2.

This is a fix.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2017

This clearly aligns the behavior of the code with the documentation (and the behavior with negative argument was clearly broken). However, this begs the question of why this limit of 2 was chosen. Would it be possible to document the motivation behind this choice?

@rbonichon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

2 is just the first number greater than 1: that's the main motivation.
Less than 2 and you basically have no indentation to speak of. I guess any other small number could be fine (3 or 4). 2 just feels like the less restrictive option.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2017

Less than 2 and you basically have no indentation to speak of

This is a motivation, we could expose it to users with some rewording of

Nothing happens if [d] is smaller than 2, to guarantee a minimal indentation.

Also, this needs a Changes entry, since it is a user-facing change.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

What is the status of this PR? Do you have the time to update it? Or should I take care of the changes bookeeping and opens another PR for the documentation?

@pierreweis

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

There is no time constraint, I merely wanted to check that this bug fix is not slipping into oblivion.

A documentation update would be nice; I may not be completely convinced that it needs to be bundled with the current bug fix; but it is fine if you wish to do so.

@pierreweis

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

@Octachron
Copy link
Contributor

left a comment

I propose to merge the current fix and I will add the corresponding documentation at a later date.

@damiendoligez damiendoligez added the bug label Nov 9, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

AFAICT all that's missing is a Changes entry.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

@rbonichon could you add a Changes entry?

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

@rbonichon , the PR needs to be rebased too (the Changes file is a bit fickle). If you don't have the time, I could do the rebase myself and force-push to your branch.

@rbonichon rbonichon force-pushed the rbonichon:fix/set_max_indent branch from 3d57c72 to aa97ff1 Jan 16, 2019

rbonichon added some commits Dec 11, 2017

[format] Make the code respect the documentation for set_max_indent
Nothing should happen when new maximum indentation is smaller than 2.
@rbonichon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@Octachron @damiendoligez I've just rebased the modifications for this PR. Does this look good to you ?

@Octachron Octachron merged commit f154c25 into ocaml:trunk Jan 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

The entry is not exactly in the right place, but some reordering was warranted anyway (I imagine that I just volunteered for doing it?). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.