RFE: Add "bst fmt" (or similar) to rewrite .bst files to canonical format

[BuildStream-Migration-Bot]

See original issue on GitLab
In GitLab by [Gitlab user @jjardon] on Jul 19, 2018, 23:47

Background

This can be useful to keep same style format in all the files in the project

Task description

This is actually done already when running bst track

Possible solutions

We have simply used this in https://gitlab.com/freedesktop-sdk/freedesktop-sdk/merge_requests/626

#!/usr/bin/python3

from ruamel.yaml import round_trip_load, round_trip_dump
    
import sys
    
for filename in sys.argv[1:]:
    print(filename)
    with open(filename) as f:
        content = round_trip_load(f)
    
    with open(filename, 'w') as f:
        round_trip_dump(content, f)

Acceptance Criteria

New command is available

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @bochecha] on Jul 19, 2018, 23:54

I like how rustfmt does it:

        --check         Run in 'check' mode. Exits with 0 if input if
                        formatted correctly. Exits with 1 and prints a diff if
                        formatting is required.

This is very useful, some projects put that in their CI so that it refuses merge requests which aren't formatted correctly.

Without this option, rustfmt just rewrites the files.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @jjardon] on Jul 20, 2018, 00:11

For reference, terraform has something similat:

• terraform fmt: format to canonical format
• terraform verify: check syntax

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Jul 20, 2018, 09:29

This is actually done already when running bst track

Is it ? That would seem to me to be a bug, we use ruamel.yaml round tripping explicitly to ensure that we do not change any whitespace or ordering or anything besides the thing we intend to change.

I think the feature request is a good one, but if we're accidentally doing this at bst track time, that seems problematic to me; even if the format does not adhere to some yet-to-be-defined standard; bst track should not introduce diffs which are unrelated to the changes it makes.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @jjardon] on Jul 20, 2018, 14:40

Is it ? That would seem to me to be a bug, we use ruamel.yaml round tripping explicitly to ensure that we do not change any whitespace or ordering or anything besides the thing we intend to change.

It is! Yeah I think it should not happen when you run bst --track, but that is #470

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @toscalix] on Jul 25, 2018, 11:48

marked this issue as related to #487

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @toscalix] on Jul 25, 2018, 11:49

removed the relation with #487

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @jjardon] on Sep 12, 2018, 12:09

marked this issue as related to #638

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @jjardon] on Oct 23, 2018, 14:34

Update on this: As pointed out at https://gitlab.com/BuildStream/buildstream/issues/470#note_97599171, bst track still changes the indentation of the bst files, so I guess we can use that to implement this?

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @jjardon] on Oct 24, 2018, 16:53

mentioned in merge request freedesktop-sdk/freedesktop-sdk!626

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @jjardon] on Oct 30, 2018, 13:56

changed the description

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @jjardon] on Oct 30, 2018, 17:16

mentioned in merge request freedesktop-sdk/freedesktop-sdk!641

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @coldtom] on Nov 13, 2018, 18:47

assigned to [Gitlab user @coldtom]

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @coldtom] on Nov 15, 2018, 16:55

mentioned in merge request !955

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Dec 3, 2018, 09:44

I think we all agree that this would be a useful feature.

I am a bit concerned about the implementation in !955, and think we need a more hands on approach to ordering of the output.

For instance, I think we can all agree that the first thing which should appear in a .bst file is the kind: attribute.

The jhbuild2bst script we used when converting jhbuild modulesets (which happens to be python2 based), shows an example of how to construct a yaml dumper with ruamel.yaml and dictate the output order of keys.

Something like this should be employed to allow control on what the default order is supposed to be. Further than this, it can be desirable to also allow plugins to contribute to informing BuildStream on what the ordering should be, since plugins also play a big part in defining the format itself.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @coldtom] on Dec 13, 2018, 11:30

I'm currently looking into doing this via plugins themselves defining the order by a keyorder attribute. The reordering functionality currently works as intended with this approach, but in overwriting the mapping representer I seem to have inadvertently broken roundtripping comments.

In more detail, I have added a keyorder attribute to elements and sources, which may be extended/overwritten by subclasses of elements/sources to include the keys necessary in the correct order. If a key is missed, the order defaults to alphabetical. This seems sensible to me, giving us a canonical order that is defined for each plugin by its author.

Edit: It turns out the comment stripping is an issue even with ruamel's own dumper, so I think this is a bug in ruamel and beyond my control here.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Jan 2, 2019, 21:47

Round tripping comments certainly work, but ruamel.yaml doesnt support them in every case I think (for example, it might forget about comments in between list items).

Also I wonder if the "mapping representer" is the representer you want to override, and not a round tripping version of the mapping representer (I think ruamel.yaml subclasses the representer for the round tripping).

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @coldtom] on Jan 24, 2019, 17:07

The comments being stripped seem to be end-of-line on list items. The representer subclassing is handled by using subclassing BstFormatter from RoundTripDumper, which handles the subclassing of representers as far as I can tell.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @jjardon] on Dec 9, 2019, 09:14

marked this issue as related to freedesktop-sdk/freedesktop-sdk#508

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on May 5, 2020, 10:16

marked this issue as related to #1299

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @coldtom] on May 5, 2020, 14:44

unassigned [Gitlab user @coldtom]