macisamuele / language-formatters-pre-commit-hooks

Collection of custom pre-commit hooks.
Apache License 2.0
116 stars 58 forks source link

Feature request: ini files with space indent #100

Closed lcnittl closed 2 years ago

lcnittl commented 2 years ago

Thanks for providing these nice hooks!

I was wondering, whether you would be willing to add an argument to the pretty-format-ini hook, that would allow to have spaces, rather than tabs for indents.

The reason for this is that many tools for checking and fixing config files would insert spaces (eg setup-cfg-fmt, tox-ini-fmt), ending up in a tab→space→tab (or space→tab→space) loop.

Thanks for your consideration, lcnittl

macisamuele commented 2 years ago

@lcnittl I'm not sure I would have the bandwidth to look into it, but PR are always welcome. If you feel like it it a feature nice to have on the tool, I'll be more than happy to have it merged.

macisamuele commented 2 years ago

@Delgan could be possible to eventually extend config_formatter to support indentation via \t instead of spaces? Assuming that we could do it then it should not be hard to expose the feature into the pre-commit hook

Delgan commented 2 years ago

@macisamuele Problems come with multi-line values:

[section]
multiline = text that spans
            on several lines
            is properly aligned.

It won't be possible to align it properly using only tabs. This leaves four possibilities:

I'm not a big fan of the two first options as mixing tabs with spaces is not very good practice and can result in misaligned values anyway (What are the downsides of mixing tabs and spaces?). The third possibility would cause the configuration before and after formatting to be parsed differently by configparser (due to the leading newline which isn't trimmed) which is undesirable I think. The last option is the more adequate solution, I guess, but it's a pity because well aligned text blocks looks nicer in my opinion (although indentation not being a multiple of 4 is a bit weird).

So basically, if it was just me, I would follow the path of black and say that config_formatter is an opinionated formatter which decided to unify formatting using spaces. :smile:

It doesn't matter much for such a tiny project, so actually I can implement the solution your prefer if you think this is important.

macisamuele commented 2 years ago

you do raise very good points 😅

let me think a bit more about it having customisation is good but it does come to a cost of maintaining code and having style discussions that the pre commit hooks were aiming to reduce.

macisamuele commented 2 years ago

I checked this again and

  1. visual indentation would depend on your editor configuration (tab is equivalent to 2 spaces, 4 spaces, n spaces)
  2. general mixing of tabs and spaces might lead to unexpected results as parsing of the INI file might lead to different assumptions whether the indentation is space or tab

If you can provide a valid use-case where this feature is needed please feel free to re-open the issue and we can re-think about this. But without further understanding on why we would like to support this I would agree with @Delgan insights and I would rather prefer not to support custom indentation style within the tool.

cc: @lcnittl