Re: [GNC-dev] gnucash maint: Multiple changes pushed

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
Op dinsdag 26 februari 2019 11:24:26 CET schreef Christopher Lam:

> Updated via  https://github.com/Gnucash/gnucash/commit/d980bb50 (commit)
> via  https://github.com/Gnucash/gnucash/commit/0ce6999f (commit)
> via  https://github.com/Gnucash/gnucash/commit/5cdc5158 (commit)
> from  https://github.com/Gnucash/gnucash/commit/316a22a2 (commit)
>
>
>
> commit d980bb50f7353a16cc4d144be89b140e55eab1d6
> Author: Christopher Lam <[hidden email]>
> Date:   Tue Feb 26 14:00:15 2019 +0800
>
>     [report] display gnc-error-dialog only when UI is running
>
>     This is preparation work for creating report.scm tests.
>
> diff --git a/gnucash/report/report-system/report.scm
> b/gnucash/report/report-system/report.scm index 017dcd347..bace494d6 100644
> --- a/gnucash/report/report-system/report.scm
> +++ b/gnucash/report/report-system/report.scm
> @@ -398,8 +398,10 @@
>          )
>          report-id
>        )
> -      (begin
> - (gnc-error-dialog '() (string-append "Report Failed! One of your
> previously opened reports has failed to open. The template on which it was
> based: " template-name ", was not found.")) +      (let ((errmsg
> (string-append "Report Failed! One of your previously opened reports has
> failed to open. The template on which it was based: " template-name ", was
> not found."))) + (if (gnucash-ui-is-running)
> +            (gnc-error-dialog '() errmsg)
> +            (gnc:warn errmsg))
>   #f))
>    )

Any reason why you didn't replace gnc-error-dialog with gnc:gui-error instead
of adding these conditionals ?

Regards,
Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Christopher Lam
Hi Geert,
Sharp eye indeed.
I haven't used the new gnc:gui-error function because report.scm is being
simulateously attacked:
(1) refactoring in maint-scheme-progress
(2) slowly creating nearly 100% coverage for tests
(3) fixing an invalid code path introduced about 8 months ago, which was
returning #f to signify failure for a report-definition-without-guid, when
in reality the report-definition-without-guid was correctly handled by
autogenerating guid.
I considered gnc:gui-error to be a proper refactoring job belonging to (1)
in my unpushed maint-scheme-progress branch but needed to create tests
during (2) first to ensure the refactoring was safe, and the tests needed
to handle non-gui code properly.
I felt that completing (2) was more important than (1).
But you're right, we could easily have used gnc:gui-error here.

On Tue, 26 Feb 2019 at 19:38, Geert Janssens <[hidden email]>
wrote:

> Op dinsdag 26 februari 2019 11:24:26 CET schreef Christopher Lam:
> > Updated        via  https://github.com/Gnucash/gnucash/commit/d980bb50
> (commit)
> >        via  https://github.com/Gnucash/gnucash/commit/0ce6999f (commit)
> >        via  https://github.com/Gnucash/gnucash/commit/5cdc5158 (commit)
> >       from  https://github.com/Gnucash/gnucash/commit/316a22a2 (commit)
> >
> >
> >
> > commit d980bb50f7353a16cc4d144be89b140e55eab1d6
> > Author: Christopher Lam <[hidden email]>
> > Date:   Tue Feb 26 14:00:15 2019 +0800
> >
> >     [report] display gnc-error-dialog only when UI is running
> >
> >     This is preparation work for creating report.scm tests.
> >
> > diff --git a/gnucash/report/report-system/report.scm
> > b/gnucash/report/report-system/report.scm index 017dcd347..bace494d6
> 100644
> > --- a/gnucash/report/report-system/report.scm
> > +++ b/gnucash/report/report-system/report.scm
> > @@ -398,8 +398,10 @@
> >          )
> >          report-id
> >        )
> > -      (begin
> > -     (gnc-error-dialog '() (string-append "Report Failed! One of your
> > previously opened reports has failed to open. The template on which it
> was
> > based: " template-name ", was not found.")) +      (let ((errmsg
> > (string-append "Report Failed! One of your previously opened reports has
> > failed to open. The template on which it was based: " template-name ",
> was
> > not found."))) +      (if (gnucash-ui-is-running)
> > +            (gnc-error-dialog '() errmsg)
> > +            (gnc:warn errmsg))
> >       #f))
> >    )
>
> Any reason why you didn't replace gnc-error-dialog with gnc:gui-error
> instead
> of adding these conditionals ?
>
> Regards,
> Geert
>
>
> _______________________________________________
> gnucash-devel mailing list
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel
>
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
Op dinsdag 26 februari 2019 12:56:47 CET schreef Christopher Lam:

> Hi Geert,
> Sharp eye indeed.
> I haven't used the new gnc:gui-error function because report.scm is being
> simulateously attacked:
> (1) refactoring in maint-scheme-progress
> (2) slowly creating nearly 100% coverage for tests
> (3) fixing an invalid code path introduced about 8 months ago, which was
> returning #f to signify failure for a report-definition-without-guid, when
> in reality the report-definition-without-guid was correctly handled by
> autogenerating guid.
> I considered gnc:gui-error to be a proper refactoring job belonging to (1)
> in my unpushed maint-scheme-progress branch but needed to create tests
> during (2) first to ensure the refactoring was safe, and the tests needed
> to handle non-gui code properly.
> I felt that completing (2) was more important than (1).
> But you're right, we could easily have used gnc:gui-error here.
>
Sure. That's fine. If it makes sense in your opinion you can also do a
followup commit to use gnc:gui-error instead. I can't tell, I'm not following
all your work in detail.

Regards,

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

GnuCash - Dev mailing list
In reply to this post by Christopher Lam
On 26/02/2019 11:56, Christopher Lam wrote:

> Hi Geert,
> Sharp eye indeed.
> I haven't used the new gnc:gui-error function because report.scm is being
> simulateously attacked:
> (1) refactoring in maint-scheme-progress
> (2) slowly creating nearly 100% coverage for tests
> (3) fixing an invalid code path introduced about 8 months ago, which was
> returning #f to signify failure for a report-definition-without-guid, when
> in reality the report-definition-without-guid was correctly handled by
> autogenerating guid.
> I considered gnc:gui-error to be a proper refactoring job belonging to (1)
> in my unpushed maint-scheme-progress branch but needed to create tests
> during (2) first to ensure the refactoring was safe, and the tests needed
> to handle non-gui code properly.
> I felt that completing (2) was more important than (1).
> But you're right, we could easily have used gnc:gui-error here.

I love your confidence, Cristopher Lam.

--
Wm


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel