Re: End of 2016 update on PyKF5 bindings

Hi Steve,

On 5 January 2017 at 22:50, Stephen Kelly <[email protected]> wrote:
>
> Shaheed Haque wrote:
>
> >> > There is also one
> >> > change which needs to be made in a synchronised fashion between
> >> > extra-cmake-modules and kguiaddons, any suggestions on how to handle
> >> > that are welcome.
> >>
> >> Sure, if you post it, we can think about how to do it. I only pushed the
> >> kguiaddons bindings today. I am curious because the patch was very small
> >> and
> >> I wonder what could possibly need to be done in concert.
> >>
> >
> > I misspoke, it wasn't in kguiadddons, but basically the ModuleCodeDb dict
> > was documented as being keyed by the filename, but I implemented it
> > wrongly, and you used a key as per the implementation, not the docs :-).
> > We can deal with the conflict, by temporarily duplicating the dict entry,
> > ahead of the core logic changing, and then removing the original, but it
> > all just needs to be done in order, perhaps with a couple of days between
> > each change to avoid tripping folk up.
>
> For simplicity, I didn't even add the ModuleCodeDb to the KDE repos yet. If
> you see that, you are probably looking at my github. My github repos are
> obsolete - they contain only a proof-of-concept. The real thing is in the
> KDE repos.

I confirm that having rebased, I'm now following KDE master exclusively.

> It is my intention to add that and other databases which are currently
> 'missing', when
>
>  * We have some tests for them in the ECM repo
>  * We want to add bindings to a framework (in a KDE repo) which requires
> them

To your first point, the bulk generator/compiler tools *are* test
tools as far as KF5 is concerned...they happen not to fit the
repository-aligned model because for my present purposes, I want to be
able to easily run them en-masse across /usr/include/KF5.

As to the second point, as mentioned, the whole point of the bulk
stuff is to make sure the tooling is up to handling the problems
before you encounter them and /usr/include/KF5 needs this stuff (I
also want the tools to be able to handle cases other than KF5 in due
course). So I prefer to not split the code along the lines you
suggest: the rebasing work that stores up for me is a huge burden.

>  I'm looking through the branches/commits in your github repo to see what can
> be applied easily now. It looks like your KDE_fix_parameter_initialisation
> fixes kguiaddons for you, however, I can build kguiaddons with no problem
> without that patch. Can you double-check, and add a unit test in ECM if you
> can confirm this needs to be done?

Please don't take stuff from commits/branches I've indicated are not
ready as I am actively rewriting them! And as above, especially let's
not split them.

I'll look and see if I can create a test case but as per my response
on the review, this is definitely a problem for me. I'll report back
via the review.

> I also saw that you have a change to how the rules engine is populated
> (add_rules method). Could you make a commit for that based on master, and
> port the ECM test to use it?

For me, to avoid creating a bunch of rebasing effort, we need to stick
to the order I outlined. If we can get review '759 and '763 committed
(separately, let me try to progress '760 as above), then I'll get
changes #1,#2,#3 as per my notes posted for review and committed. That
will get you what you want, and the rest can be on a private branch. I
can then easily cherry-pick the tiny changes to rules_engine.py and
sip_compiler.py from the later commits for the master branch.

> > My *main* regression tests therefore include carefully diff'ing the 2280
> > files from run to run, gradually picking off errors as I can to get to then
> > point of a clean SIP compiler run across the 133 components in turn. (BTW,
> > that is also the reason for the continued existence of
> > sip_buik_generator.py and sip_compiler.py as necessary tooling to support
> > this development and testing). It was this test process that found this
> > bug when an assert was triggered.
>
> Sounds like an interesting system.

It is a bit of a brute force approach, but I believe it represents the
biggest bang for my buck in making the system robust. and its what has
got us this far...(not to diminish what you have done!).

> > Now, if I could easily see what in the source code of the enum causes
> > libclang to emit an attribute, I would have considered writing the test,
> > but based on the ROI of the effort involved in doing what you suggest
> > (which is, I agree, theoretically the right thing to do) just did not meet
> > the bar of a simple-to-develop test, given the scope of what else I need
> > to get done. That is, not firing on the assert seemed, and frankly seems,
> > good enough to me in this case.
>
> I guess we got this item sorted out already now anyway. Often, once the fix
> exists, the test is obvious and quick to write.
>
> > Excellent. Having had a quick look at the code in the KDE repos, I see
> > that there are quite some changes, notably to rules_engine.py which
> > removes some of the test facilities I need to revive the
> > sip_builk_generator.py test workflow. I'll see if I can package those
> > changes with collateral that will ease any concerns with the testing that
> > is being done.
>
> I'd like to keep the bulk generator out of the KDE repo for now, as it won't
> be needed to build frameworks repos.

As above, I've put it in a subdirectory to keep it out of the way, and
if needed, this can stay on a branch or something. But I do want to
get it into the same repo as the sip_compiler.py if at all possible.

How does that sound?

> Thanks,
>
> Steve.