[Aptitude-devel] Improvement/simplification of the implementation of parsing sorting policies…

[Daniel Hartwig]

On 27 February 2012 18:19, Manuel A. Fernandez Montecelo
<manuel.montezelo at gmail.com> wrote:
> 2012/2/27 Daniel Hartwig <mandyke at gmail.com>:
>> Even though no existing sorting policies made use of arguments and
>> removing them has made it easier for you to make use of tokenizer, it
>> is worth keeping the feature:
>>
>>  - the user manual states sorting policies share the same syntax
>>    as grouping policies, which permit and use arguments;
>>  - it is not causing any problems at the moment;
>>  - future sorting policies may make use of it: consider a by-arch
>>    policy which permits the user to specify some important arch(s)
>>    to sort before others;
>>  - if this is added back in the future (very likely), then the
>>    translations of messages ("By-foo sorting polices takes no
>>    arguments.") will have been lost in the meantime, generating
>>    extra work for translaters.
>>
>> Had the original parser been problematic, I'd say this was a good
>> choice (remove the problems and some disused features).  But since
>> it was not problematic it is prefered to leave the code as it is,
>> with the potentially useful features in place.
>>
>> Lastly, the ideal, final outcome outcome of any refactoring of the
>> sorting or grouping policy parsers is to unite these two pieces of
>> code since the format is identical between them.  There is this
>> comment by D. Burrows to back up this statement:
>>
>> -- load_sortpolicy.cc
>> //  Loads sorting policies -- similar to grouping policies.  (in fact, I did
>> // this with cut&paste, and think maybe I should consider trying to share
>> // code between the two parsers..)
>
> I disagree.  So the way to produce the original code is fundamentally
> the wrong way to do it, acknowledged by the original author.  However
> this wasn't fixed for 6 years.  The whole file was unchanged since
> those days, no new policy was introduced whatsoever in the ~6 years.

Nothing changed in this file because the development was occuring on
the more complex parser (load_grouppolicy).

The problem noted in that comment is that these two files should be
sharing code.  Other than that the existing parser worked perfectly
fine.  This is why I don't see *some* of the changes made to be that
useful (such as the switch from while- to for-loop at the end, and
identifier renaming).


>> D. Burrows has been working on making improvements to this code,
>> including making it more generic.  This was being developed in
>> load_grouppolicy.cc ("git log src/load_grouppolicy.cc").  That code is
>> a fair bit more complicated, but any attempt to bring these two
>> parsers together should definitely start from there instead of
>> load_sortpolicy.cc.
>
> My change, stripping the sorting policies and leaving a trivial amount
> of mostly empty classes and trivial parser, actually paves the way for
> refactoring the whole bunch of files to use a common method, seeing
> that the conversion to anything else is equally trivial.
>

I agree that it does.  Approaching the task from either side is valid.
Perhaps my wording was off, but I did not mean to imply otherwise --
rather that the task is already begun in load_grouppolicy and that,
IMO, this is the logical place to resume it.

However, given that it is not me attempting this, but yourself, you
are clearly free to persue it any way you deem appropriate.  And I
certainly encourage you to continue to do so if that was your
intention.


> But I guess that you disagree, so sorry for that.
>
>> For these reasons alone I seriously think it is worth reverting this
>> change completely.
>
> OK, go ahead then.
>

Well, not if you intended to continue the work to unify the two
parsers -- that is a highly desirable goal.

As I said, it is your work and you are directing it, my comments aim
only to point out some issues that *I* perceived -- the change
functions perfectly fine, so there are no technical reasons for you
not to continue on it.

If we do not mutually check up on each others' work then we risk some
problems arising that are difficult to deal with down the track.

I urge you not to be discouraged as that was not my intent :-/


>> If you have not done so already, I would strongly suggest becoming
>> familiar with all of those files before continuing to develop major
>> changes in aptitude.
>
> OK, I will refrain from develop major changes in aptitude then.
>

Please do not take such a drastic step over relatively minor
differences in coding style.

>From looking at this commit it is clear that your personal coding
style differs from that used by the existing code -- mine does as
well.  The differences are small but quite noticable.  As the existing
code is mostly consistent and some direction is explicitly mentioned,
e.g., in README.NAMING, does it not make sense to develop new code in
this same style?

Really the relevant parts of those README* files are not that large
and the coding style in use is immediately obvious in any file.  It
is not a big task to identify and use that style.

Most significantly sized projects encourage--even require--such
adherence to a particular coding style.