[yast-devel] The Business Case for Y-Storage - Proposal vs. Device Graph
Following up on the "Refactoring madness" thread and an IRC discussion;
I had written lately:
"this is an approach I don't like at all because it focuses on the wrong
thing: All that stuff is not about creating device graphs. We are not in the
business of making or selling device graphs. Device graphs are just a tool
for what we really intend to do. Concentrating on an irrelevant (or, to put
it less harshly, behind-the-scenes) tool is most misleading for anybody
looking at that code."
I was asked to elaborate that point further, so here we go:
What is the point of what we are doing in that new yast-storage-ng? This is
really all about making a storage proposal for our installation workflow;
this is the main purpose of it all, with more specialized issues like the
expert partitioner aside.
In most of the history of YaST2, we had the concept of providing the user
with a useful, reasonable "proposal" for each aspect of the Linux
installation. It was always intentionally called "proposal" because it is not
set in stone; the user can always change it if it doesn't fit his needs.
So we've been doing some calculations based on hardware probing, based on
product parameters (control.xml), based on guessed values for every aspect of
the installation: For the bootloader, for the software selection, for the
desktop etc. etc., and for storage: What partitions to remove or to resize to
make room for Linux, what new partitions and file systems to create, where to
mount them.
So the term "proposal" is a well-known concept in the YaST installation for
YaST developers as well as for other people in SUSE R&D, including product
and project managers, and even for some advanced users.
So, when looking through the installation code, I would expect that to be
somehow reflected in the code; I'd expect a class "StorageProposal"
somewhere, preferably with a main method 'propose()'. And it used to be like
that in my first version of that new storage proposal.
But not for long: It was changed very soon to a class "DevicegraphGenerator".
There is now also a class "RefinedDevicegraph". One thing that is no longer
present, though, is anything with "Proposal" in the name.
To me, this is not only not self-explanatory (as good code should be), it is
outright confusing. Anybody searching for the place where a storage proposal
is made will have to do a lot of grepping to find where it happens, and so
far every was vers much surprised (to put it mildly) where this was.
That's misleading IMHO. Device graphs are a useful tool in that context, but
just that: A tool. They are not the central part around everything revolves,
at least not from the perspective of a developer using that part.
That's why I had written that we are not in the business of making or selling
device graphs, we are in the business of making useful proposals. I miss the
whole concept of proposals here. There is only one function in the
DevicegraphGenerator called proposal(), and even that was only named like
this after several people had agreed that run() is not a good name, in
particular when you have a run() function in several classes (good luck
finding the right one with 'grep'!).
So, what we are doing now is to put very much emphasis on an implementation
detail while omitting the well-known concept.
Kind regards
--
Stefan Hundhammer
On Wed, 2 Mar 2016 17:43:36 +0100
Stefan Hundhammer
Following up on the "Refactoring madness" thread and an IRC discussion; I had written lately:
"this is an approach I don't like at all because it focuses on the wrong thing: All that stuff is not about creating device graphs. We are not in the business of making or selling device graphs. Device graphs are just a tool for what we really intend to do. Concentrating on an irrelevant (or, to put it less harshly, behind-the-scenes) tool is most misleading for anybody looking at that code."
I was asked to elaborate that point further, so here we go:
What is the point of what we are doing in that new yast-storage-ng? This is really all about making a storage proposal for our installation workflow; this is the main purpose of it all, with more specialized issues like the expert partitioner aside.
In most of the history of YaST2, we had the concept of providing the user with a useful, reasonable "proposal" for each aspect of the Linux installation. It was always intentionally called "proposal" because it is not set in stone; the user can always change it if it doesn't fit his needs.
So we've been doing some calculations based on hardware probing, based on product parameters (control.xml), based on guessed values for every aspect of the installation: For the bootloader, for the software selection, for the desktop etc. etc., and for storage: What partitions to remove or to resize to make room for Linux, what new partitions and file systems to create, where to mount them.
So the term "proposal" is a well-known concept in the YaST installation for YaST developers as well as for other people in SUSE R&D, including product and project managers, and even for some advanced users.
So, when looking through the installation code, I would expect that to be somehow reflected in the code; I'd expect a class "StorageProposal" somewhere, preferably with a main method 'propose()'. And it used to be like that in my first version of that new storage proposal.
Question is if it is expected also by others as current way is having module with given name and method Propose (currently in yast code is 44 method Propose ) or client called _proposal ( even more now ). Checking for current number of Proposal class is a bit hard as it is also name of _proposal client. So e.g. myself I expect something like Storage.propose which hides details inside. Not mandatory note, just to be aware that expectation can be different.
But not for long: It was changed very soon to a class "DevicegraphGenerator". There is now also a class "RefinedDevicegraph". One thing that is no longer present, though, is anything with "Proposal" in the name.
As written above need not to be problem as long as there is something like Storage.propose. Not everything have to be name. For me Proposal class is more like functor that is more functional then object. Object is for me Storage configuration represented by class Storage, that can be read/written, proposed, import/export.
To me, this is not only not self-explanatory (as good code should be), it is outright confusing. Anybody searching for the place where a storage proposal is made will have to do a lot of grepping to find where it happens, and so far every was vers much surprised (to put it mildly) where this was.
Anybody searching is a bit strong statement, do you do some User testing or research regarding it?
That's misleading IMHO. Device graphs are a useful tool in that context, but just that: A tool. They are not the central part around everything revolves, at least not from the perspective of a developer using that part.
yes, tool. For me the most important starting point of class representing Storage configuration which can contain graph holding information about configuration that can be queried. For me graph is not tool, for me it is object holding storage configuration that allows querying and modification of that configuration. To be honest I do not think it need to contain graph word in it, but currently don't have better name for it.
That's why I had written that we are not in the business of making or selling device graphs, we are in the business of making useful proposals. I miss the whole concept of proposals here. There is only one function in the DevicegraphGenerator called proposal(), and even that was only named like this after several people had agreed that run() is not a good name, in particular when you have a run() function in several classes (good luck finding the right one with 'grep'!).
I agree with you, that "run" is very bad name as it indicate Functor overusage indicating procedural programming. So as I said I expect something like Storage.propose that do proposal. I also agree with you that DeviceGraphGenerator is not where I start searching for proposal. Probably DeviceGraphGenerator is bad name and something like StorageConfiguration is better with method propose, system ( for system wide configuration ) and something like target_one method ( better name can be found for sure ) is what can be useful. Actually maybe not convenient for Yast, but more intuitive for me as system can do lazy "Read" and for target_one should be method write, which write that target one to system.
So, what we are doing now is to put very much emphasis on an implementation detail while omitting the well-known concept.
well known concept - I try to google "proposal concept", "programming proposal concept" or "IT proposal concept" and nothing return my anything useful. So maybe it is well known only in SUSE or maybe YaST. What is currently closes query that brings me similar to topic is "default values" which mention it. Do maybe we should stop living is small yast pond and look elsewhere what is actually well known concept. And what actually yast do is that it calculate default values for installation which is understandable even outside of yast, suse or linux world. So I fully agree that DeviceGraphGenerator.proposal or run is not nice name and I agree that some StorageProposal.run or propose is better, I still feel that it is not good enough solution as it is not intuitive as Storage.propose or StorageConfiguration.proposal which is the first idea when to search when we speak that we want to propose default values for Storage configuration. It is similar case like if we took base OOP example with cat and #say method and we propose to use something like AnimalSpeaker.say(cat) instead of cat.say Josef
Kind regards
-- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 03/02/2016 05:43 PM, Stefan Hundhammer wrote:
Following up on the "Refactoring madness" thread and an IRC discussion; I had written lately:
"this is an approach I don't like at all because it focuses on the wrong thing: All that stuff is not about creating device graphs. We are not in the business of making or selling device graphs. Device graphs are just a tool for what we really intend to do. Concentrating on an irrelevant (or, to put it less harshly, behind-the-scenes) tool is most misleading for anybody looking at that code."
I was asked to elaborate that point further, so here we go:
What is the point of what we are doing in that new yast-storage-ng? This is really all about making a storage proposal for our installation workflow; this is the main purpose of it all, with more specialized issues like the expert partitioner aside.
In most of the history of YaST2, we had the concept of providing the user with a useful, reasonable "proposal" for each aspect of the Linux installation. It was always intentionally called "proposal" because it is not set in stone; the user can always change it if it doesn't fit his needs.
So we've been doing some calculations based on hardware probing, based on product parameters (control.xml), based on guessed values for every aspect of the installation: For the bootloader, for the software selection, for the desktop etc. etc., and for storage: What partitions to remove or to resize to make room for Linux, what new partitions and file systems to create, where to mount them.
So the term "proposal" is a well-known concept in the YaST installation for YaST developers as well as for other people in SUSE R&D, including product and project managers, and even for some advanced users.
I think we all agree with the whole content up to this point.
So, when looking through the installation code, I would expect that to be somehow reflected in the code; I'd expect a class "StorageProposal" somewhere, preferably with a main method 'propose()'. And it used to be like that in my first version of that new storage proposal.
Before the refactoring, there was a class Yast::Storage::StorageProposal with a method called "propose" Now, after the refactoring, there is a class Yast::Storage::Proposal [1] also with a method called "propose" Not such a big change and certainly not a fundamental change in the whole approach. But I don't care that much about naming, if you want to bring the original name (redundant with the namespace IMO) back. It's true that in the original version of the pull request, the responsibility of calculating the proposal was directly in Yast::Storage::ProposalDemoClient#run I did it because I considered that a minor problem for the time being. The pull request was more focused in the stuff already mentioned in my previous mails, like removing the global catalog, avoiding I/O arguments, avoiding to have Storage.instance.probed all over the code and so on. Thanks god we have code reviews. You pointed out that putting that responsibility directly on the client was an error and thus we created the mentioned Yast::Storage::Proposal#propose and turned ProposalDemoClient into a more dumb class relying on it. Once again, not a change in the whole approach, just a minor responsibility relocation and more explicit method naming. The normal outcome of a code review.
But not for long: It was changed very soon to a class "DevicegraphGenerator". There is now also a class "RefinedDevicegraph". One thing that is no longer present, though, is anything with "Proposal" in the name.
DevicegraphGenerator is not a substitute for StorageProposal and never was. The already mentioned Storage::Proposal is that substitute. In the very first version of the pull request (the one that was corrected), that substitute was Storage::ProposalDemoClient. DevicegraphGenerator is a class that implements one of the 2 big steps currently included in the generation of the proposal. First we calculate the requirements (done by VolumesGenerator) and then we calculate a devicegraph to fulfill those requirements (done by DevicegraphGenerator). On the other hand, yes, there is now a refinement called Yast::Storage::Proposal::RefinedDevicegraph [2] ( That module contains several methods to query the status of a given devicegraph. Those methods were previously spread over classes like DiskAnalyzer and SpaceMaker.
To me, this is not only not self-explanatory (as good code should be), it is outright confusing. Anybody searching for the place where a storage proposal is made will have to do a lot of grepping to find where it happens, and so far every was vers much surprised (to put it mildly) where this was.
Why is it more self-explanatory to find those proposal-related methods in SpaceMaker or in DiskAnalyzer (two files located in /lib/storage, if we talk about grep) than in Proposal::RefinedDevicegraph (a file located in /lib/storage/proposal)? Or do you mean that Storage::Proposal#propose is outright confusing compared to Storage::StorageProposal#propose? Sure Storage::ProposalDemoClient#run is less obvious (if you insist in discussing a code that was corrected before the merge), but far from being a fundamental problem in the whole approach. Just a problem corrected with better names.
That's misleading IMHO. Device graphs are a useful tool in that context, but just that: A tool. They are not the central part around everything revolves, at least not from the perspective of a developer using that part.
I get the main idea of not exposing devicegraph. But I don't get in which way the pre-refactoring code was better in that regard. Yes, we now have a Proposal:RefinedDevicegraph module that other classes has to know about. But before the refactoring those classes already had a dependency on Devicegraph (they manipulate devicegraphs after all) and IN ADDITION they had dependencies on SpaceMaker and DiskAnalyzer. There is not a single class in your examples (and in the whole code, I would say) that knows now about devicegraphs and didn't know about them before the refactoring.
That's why I had written that we are not in the business of making or selling device graphs, we are in the business of making useful proposals. I miss the whole concept of proposals here. There is only one function in the DevicegraphGenerator called proposal(), and even that was only named like this after several people had agreed that run() is not a good name, in particular when you have a run() function in several classes (good luck finding the right one with 'grep'!).
Once again. You are focusing the whole discussion on a set of bad names that were present in the original version of a pull request claiming that it shows fundamental differences in the approach. I offered ZERO resistance to change those names and relocate those methods. That very first version of the pull request touched around 1,000 lines of code in 20 different files. It did so to fight against all the problems described in my mails (singleton abuse, global catalog, I/O arguments, single responsibility...). Those are fundamental differences in the approach, from my POV.
So, what we are doing now is to put very much emphasis on an implementation detail while omitting the well-known concept.
Yes, apart from the already mentioned relevant changes. The original version of the pull request also contained some bad names that were corrected during code review (add JUST another 1 file and another 30 lines of code to the above-mentioned huge numbers). Focusing the discussion on that is looking at the finger rather than the at the moon. Claiming that those bad names reveal fundamental differences is making a mountain out of a molehill... and what is worse, ignoring the real mountain whatsoever.
Kind regards
Not so fast! (sorry, I always have to win the verbosity contest)... ;-) The statement I asked you to elaborate (you know, "this is an approach I don't like at all because it focuses on the wrong thing...") was not written as a reply to the refactoring pull request we have ended up discussing in this mail. It was written as a reply to my claim that this solution (proposed by me) factory = Yast::Storage::DevicegraphFactory.new devicegraph = factory.create_from_file(input_file) devicegraph.serialize(output_file) was better than this one (present in the current code) fake_probing = Yast::Storage::FakeProbing.new devicegraph = fake_probing.devicegraph Yast::Storage::FakeDeviceFactry.load_yml_file(devicegraph,input_file) Yast::Storage::YamlWriter.write(devicegraph, output_file) Sorry to ask again (and we should probably move the discussion out of the mailing list now) but I still don't see how the second option is better at hiding the devicegraphs as an implementation detail to the outside world. In the same way I still don't see why the pre-refactoring code was better than the refactored one in that regard. But as said, maybe it's time to continue the discussion "live". Cheers [1] https://github.com/yast/yast-storage-ng/blob/master/src/lib/storage/proposal... [2] https://github.com/yast/yast-storage-ng/blob/master/src/lib/storage/proposal... -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 03/03/2016 01:43 AM, Ancor Gonzalez Sosa wrote:
[Stripped a huge text]
I realized my text is too large and focused on too much concrete stuff. It needs a TL;DR so the main point is more clear. Here we go: My original refactoring of the proposal was intended to solve a number of problems. In its original shape, it also introduced the new problem exposed by HuHa in this thread. Let's call it "problem X". That problem was pointed during the code review and fixed before the merge. I don't get the relation between problem X and other problems I have pointed in other code reviews and in my mails. I don't get why the existence of problem X at a given point in time should stop us from discussing other problems. Moreover, the fact that problem X was detected and solved during a code review is, IMHO, an argument to keep discussing design stuff in code reviews. And I have the feeling that it has, to the contrary, become an excuse to avoid discussions on completely unrelated stuff. Cheers. -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
participants (3)
-
Ancor Gonzalez Sosa -
Josef Reidinger -
Stefan Hundhammer