WIP: bst fmt: Add basic functionality #1415
Conversation
Adds basic functionality for `bst fmt`, formatting the yaml files of elements TODO: * Weird issue in yaml dumping - see #767 * Add a check option, which just checks if format correct * Add a canonical order of top-level nodes on a per project basis
Allow authors of plugins to utilise the Plugin.keyorder attribute to define an order for the sub-headers in config. Modify `bst fmt` to modify the yaml into that order. Add canonical keyorder for every plugin already in core. In terms of implementation, this adds a custom yaml dumper subclassed from ruamel.yaml's RoundTripDumper, which will dump a dict in the order defined in the keyorder attribute. The Plugin class is given a keyorder attribute, which defines the order in which keys should be dumped. The element and source classes add the common fields used in them, so as to minimise work for plugin authors. Plugin authors should add their custom keys at configure time. This causes some stripping of comments, which seems to be due to a bug in the ruamel.yaml RoundTripDumper, as that also strips the same comments. It also will remove blank spaces, again probably due to limitations in ruamel.
BuildStream-Migration-Bot
requested review from
BenjaminSchubert,
cs-shadow,
gtristan and
juergbi
as code owners
|
In GitLab by [Gitlab user @jjardon] on Nov 15, 2018, 17:14 changed the description |
|
In GitLab by [Gitlab user @coldtom] on Nov 15, 2018, 17:59 added 1 commit
|
|
In GitLab by [Gitlab user @knownexus] on Nov 19, 2018, 14:21 Commented on buildstream/_frontend/cli.py line 374 Shouldn't there be a requirement for --all to be enabled before we allow --except? |
|
In GitLab by [Gitlab user @knownexus] on Nov 19, 2018, 14:25 Commented on buildstream/_scheduler/queues/formatqueue.py line 12 Is this called anywhere? I can't see it but i may be missing it |
|
In GitLab by [Gitlab user @coldtom] on Nov 21, 2018, 12:56 Commented on buildstream/_scheduler/queues/formatqueue.py line 12 As I understand it this is where one implements the process performed by the queue, and is called by the scheduler. It's an abstract method in the |
|
In GitLab by [Gitlab user @coldtom] on Nov 21, 2018, 13:47 Commented on buildstream/_frontend/cli.py line 374 Other elements with similar functionality use the same method of implementing |
|
In GitLab by [Gitlab user @tlater] on Nov 21, 2018, 16:16 Commented on buildstream/_scheduler/queues/formatqueue.py line 12 [Gitlab user @knownexus] it's part of the |
|
In GitLab by [Gitlab user @tlater] on Nov 21, 2018, 16:19 Commented on buildstream/_stream.py line 297 While I understand that you're just reusing code here, I'd prefer if you changed the name of the |
|
In GitLab by [Gitlab user @tlater] on Nov 21, 2018, 16:26 Commented on buildstream/_frontend/cli.py line 374 I think that the |
|
In GitLab by [Gitlab user @tlater] on Nov 21, 2018, 16:27 Commented on buildstream/_scheduler/queues/formatqueue.py line 15 I'd be pretty impressed if you could do this. Any suggestions on how this would be implemented? Surely you'd run into the halting problem here. |
|
In GitLab by [Gitlab user @tlater] on Nov 21, 2018, 16:38 So, the ML was pretty happy with this, but as much as I see it being useful, I worry about feature creep. We have been adding a lot of new commands to BuildStream in this cycle, but they have mostly been there to expose missing bits of API and clean up inconsistencies. This adds an entirely new feature (tracking partially does this, but that's by accident, not design). I would be unhappy landing this without having [Gitlab user @tristan] or [Gitlab user @juergbi] at least approve the feature (neither of them seems to have replied to the ML topic). Before we land this we should probably also define what "canonical" means, document it, and ensure that our implementation here actually applies that format through tests. I doubt that it currently is particularly stable, or matches what the larger yaml community thinks should be "canonical". In my mind this aims to be an opinionated formatter, and if we're writing one of those, we should do it right. Hence I also think that this entire feature probably should be a separate project. Also, on that note, please add some tests beyond ensuring that completion works. |
|
In GitLab by [Gitlab user @coldtom] on Nov 22, 2018, 09:48 I understand concerns about feature creep, I don't think BuildStream would need this feature if not for Currently canonical format is "whatever the ruamel.yaml dumper spits out by default" but this could be configured to be something defined and consistent. The format we consider "canonical" will have to be consistent with what the dumper does when run, otherwise we will have the same issue when running I'm not opposed to the formatter being a separate tool, but if it is we need to document that |
|
In GitLab by [Gitlab user @coldtom] on Nov 22, 2018, 09:54 Commented on buildstream/_scheduler/queues/formatqueue.py line 15 While considering the implementation details I realised that this would offer little to no optimisation given the feature as it stands. The quickest way I can think of to check if an element is formatted correctly is to dump the node and compare the files, but obviously this is slower than just dumping the node. |
|
In GitLab by [Gitlab user @coldtom] on Nov 22, 2018, 16:44 Commented on buildstream/_stream.py line 297 changed this line in version 3 of the diff |
|
In GitLab by [Gitlab user @coldtom] on Nov 22, 2018, 16:44 Commented on buildstream/_scheduler/queues/formatqueue.py line 15 changed this line in version 3 of the diff |
|
In GitLab by [Gitlab user @coldtom] on Nov 22, 2018, 16:44 added 74 commits
|
|
In GitLab by [Gitlab user @tristanvb] on Dec 3, 2018, 09:45 Commented on buildstream/element.py line 1342 This probably needs to have a lot more control over the ordering of keys in the output. I.e. see https://gitlab.com/BuildStream/buildstream/issues/485#note_121782763 |
|
In GitLab by [Gitlab user @coldtom] on Dec 13, 2018, 11:31 marked as a Work In Progress |
|
In GitLab by [Gitlab user @coldtom] on Dec 13, 2018, 13:46 added 157 commits
|
|
In GitLab by [Gitlab user @coldtom] on Dec 17, 2018, 12:37 added 15 commits
|
|
In GitLab by [Gitlab user @coldtom] on Dec 20, 2018, 11:41 added 12 commits |
|
In GitLab by [Gitlab user @coldtom] on Dec 20, 2018, 11:43 Commented on buildstream/element.py line 1342 [Gitlab user @tristanvb] I have reworked this to have greater control over the order of dumping, but it has introduced an issue in stripping comments in some situations. I still need to add tests but want feedback on the approach used first. |
|
In GitLab by [Gitlab user @LaurenceUrhegyi] on Mar 26, 2019, 18:15 Hi, thanks for the contribution! Unfortunately this WIP MR is over a month without an update from the author, so as per our policy I'm going to close this to keep our queue tidy. Please do re-open the MR if you come back to work on it. Cheers! |
|
In GitLab by [Gitlab user @LaurenceUrhegyi] on Mar 26, 2019, 18:15 closed |
|
In GitLab by [Gitlab user @jjardon] on Mar 26, 2019, 19:32 This MR is waiting for feedback from the maintainers; reopening |
|
In GitLab by [Gitlab user @jjardon] on Mar 26, 2019, 19:32 reopened |
|
In GitLab by [Gitlab user @jjardon] on May 4, 2019, 03:54 [Gitlab user @coldtom] can you please rebase this? For the comments seems It's (almost) ready to merge |
|
In GitLab by [Gitlab user @danielsilverstone-ct] on Jul 4, 2019, 09:44 Hi, thanks for the contribution! Unfortunately this WIP MR is over a month without an update from the author, so as per our policy I'm going to close this to keep our queue tidy. [Gitlab user @coldtom] please re-open the MR if you come back to work on it. Cheers! |
|
In GitLab by [Gitlab user @danielsilverstone-ct] on Jul 4, 2019, 09:44 closed |
|
In GitLab by [Gitlab user @jjardon] on Dec 9, 2019, 09:15 Again, This MR was waiting for feedback from the maintainers; reopening |
|
In GitLab by [Gitlab user @jjardon] on Dec 9, 2019, 09:15 reopened |
|
In GitLab by [Gitlab user @BenjaminSchubert] on Dec 9, 2019, 10:13 I'm not sure what we would gain with that command:
|
|
In GitLab by [Gitlab user @jjardon] on Dec 12, 2019, 04:34 [Gitlab user @BenjaminSchubert] does yamllint convert to a canonical format or only errors/warns if something is not following the format? This is about the former |
|
In GitLab by [Gitlab user @BenjaminSchubert] on May 11, 2020, 20:46 [Gitlab user @jjardon] woops seems I missed that one. No, yamllint doesn't convert to a canonical format. Seems like we'd still need that one then. |
See original merge request on GitLab
In GitLab by [Gitlab user @coldtom] on Nov 15, 2018, 16:55
Description
Adds a
bst fmtcommand, which modifies the format of a projects element files into a canonical yaml format. This has the main benefit of automating files into the formatbst trackwill dump them into when run.Currently the functionality is somewhat stinted by #767, and there are a couple of other features I would like to add to this - namely an option which doesn't modify the yaml, but creates a diff, and a way for a user to define the order of top-level nodes in a
.bstfile, into whichbst fmtsorts the nodes.Changes proposed in this merge request:
bst fmtcommand which formats elements into a canonical formatThis merge request, when approved, will close #485