| cjwatson | wgrant: Is there a good way to handle the case where setting one field in a Storm class needs to potentially adjust that value depending on the value of another field? Setting git_path needs to do git_repository.getRefByPath(); but if I just do the naïve thing and write git_path.setter, I get an IntegrityError while submitting edit forms, because updateContextFromData sets git_repository and then attempts to set git_path, but the ... | 00:05 |
|---|---|---|
| cjwatson | ... implicit flush at the start of the resulting Store.find causes an INSERT with git_path = null. | 00:05 |
| cjwatson | I was looking for some kind of filtering property on the interface field but couldn't find one. | 00:05 |
| cjwatson | (i.e. so that lazr.restful could do the mangling before it got as far as updateContextFromData) | 00:06 |
| cjwatson | We have various other property setters but as far as I've found so far they aren't in situations where they need to care much about the implicit flush - the problem here is that git_repository and git_path need to be set consistently with each other, but setting the latter depends on the former | 00:08 |
| wgrant | cjwatson: Is validating that really useful? | 00:14 |
| wgrant | cjwatson: It is going to be able to drift anyway. | 00:14 |
| cjwatson | wgrant: Canonicalising is useful, because it makes it a lot easier to implement GitRef:+snaps. | 00:15 |
| cjwatson | And people are likely to enter master rather than refs/heads/master | 00:15 |
| wgrant | cjwatson: Right, but that could just be a custom interface field. | 00:16 |
| wgrant | It doesn't depend on the repository AFAICS | 00:16 |
| cjwatson | I think it would be rather helpful to say that it doesn't exist if you make a typo when you try to enter it, even if it vanishes later | 00:16 |
| wgrant | Doing that on the API is difficult. | 00:17 |
| wgrant | For the web UI I'd just have something custom in validate() | 00:17 |
| cjwatson | For form submissions I can do it in the view's validate method, and for Snap creation I can do it in SnapSet.new. Maybe I just have to sacrifice the ability to do it on the webservice. | 00:17 |
| wgrant | Correct. | 00:17 |
| wgrant | The order of operations of a multi-field patch is not well-defined, and there's no way to take an action after everything is set (other than IObjectModifiedEvent) | 00:18 |
| wgrant | And doing validation in the modified event is unprecedented AFAIK. | 00:18 |
| cjwatson | If there's some way to have single-field webservice sets go through a different method than internal sets, that would be useful | 00:19 |
| wgrant | @mutator_for | 00:19 |
| cjwatson | Oho | 00:19 |
| cjwatson | Of course. That would cover more or less all the bases then | 00:19 |
| wgrant | I don't think you can use it :) | 00:19 |
| cjwatson | Why not? | 00:19 |
| wgrant | If I'm changing the repo and path simultaneously, I may need both to change before either will validate. | 00:20 |
| cjwatson | Well, only if there's a validator on the repository | 00:20 |
| cjwatson | But yes, perhaps you're right | 00:20 |
| wgrant | It would seem unusual for it to be illegal to set the ref to something that doesn't exist in the repo, but legal to change the repo to one that doesn't have that ref. | 00:21 |
| cjwatson | I kind of want to say that this implies that only Snap.git_ref should be exposed | 00:21 |
| cjwatson | But we decided not to do that for BMP | 00:21 |
| wgrant | It was impossible for BMP as the repo references needed to remain intact even after the branch was deleted. | 00:22 |
| wgrant | Oh | 00:22 |
| wgrant | Could have done a virtual one, though, I suppose. | 00:22 |
| wgrant | I don't recall why we didn't do that. | 00:22 |
| cjwatson | I don't think I'd conceived of GitRefFrozen when we designed the BMP exports. | 00:23 |
| cjwatson | Maybe. Although I did eventually do that for BMP. | 00:23 |
| wgrant | It is indeed possible not. | 00:23 |
| wgrant | I would continue to expose the split fields directly anyway, if only read-only, for convenience. | 00:23 |
| cjwatson | There's a good reason to continue to expose them: you might have a snap that was built from a ref that has been deleted. | 00:24 |
| cjwatson | But I'm inclined to say it should only be possible to set by the combined field. | 00:24 |
| wgrant | Or we could just have a named operation that sets both. | 00:25 |
| cjwatson | GitRef is already exported, so I see no reason not to use it. | 00:25 |
| cjwatson | BMP is partially consistent with this. You can only create one using GitRef.createMergeProposal; but once you've created one, you can only modify it using the combined fields. | 00:26 |
| cjwatson | Oh, but those fields are read-only on BMP anyway. | 00:26 |
| cjwatson | You have to resubmit to change them, and resubmit isn't exposed on the webservice. | 00:26 |
| wgrant | And resubmit is a named op anyway | 00:27 |
| cjwatson | So BMP isn't as good a model as I thought it was. | 00:27 |
| cjwatson | I guess the main remaining problem is that I'd need a GitRefVocabulary. | 00:29 |
| cjwatson | Or maybe not, that field wouldn't be in the form. | 00:30 |
| cjwatson | This feels a lot nicer already. | 00:33 |
| wgrant | I dislike using object references when they are silly, but it may be best here. | 00:33 |
| cjwatson | I like it when I can delete tests for whether things are canonicalised properly because there's no way to express passing in anything non-canonical. | 00:38 |
| === G_ is now known as G | ||
| cjwatson | wgrant: Did you get a chance to look at team-mail again? | 12:14 |
| wgrant | cjwatson: It should have a vote, unless I have too many MPs open. | 12:26 |
| cjwatson | wgrant: So it does, buried in threaded mailer. Thanks. | 12:49 |
| === danilos` is now known as danilos | ||
| lifeless | cjwatson: ping (https://github.com/testing-cabal/testtools/pull/149) | 21:51 |
| cjwatson | ah, answered on #ubuntu-devel | 22:49 |
| lifeless | :) | 23:12 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!