Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing logic around testing if a template exists. #1354

Closed
wants to merge 1 commit into from

Conversation

kielstr
Copy link

@kielstr kielstr commented Jun 18, 2017

Check for error and provide feedback.

@cromedome cromedome added this to the 2017-10-13 milestone Oct 10, 2017
@cromedome
Copy link
Contributor

I normally prefer Try::Tiny to error checking of eval{}, but since we use eval{} all over the place I can't really complain 😄

However, this PR does cause t/auto_page.t to fail. I haven't had a chance to see if it's just a poorly written test, or if this change actually breaks the auto page functionality. If you have a chance to investigate and report back/fix, great. I will also try to look at this in the next day or two so we can release this ASAP.

Thanks for your PR! I like it, and will be eager to merge it once we can figure out what the auto page issue is.

@veryrusty
Copy link
Member

pathname_exists should return a false value if the file doesn't exist. This PR alters the return semantics; 1 for file exists and -1 when not existing. However -1 isn't a perl false value.

Something like the following would be more appropriate (logic wise)

sub pathname_exists {
    my ( $self, $pathname ) = @_;

    my $exists = eval {
        # dies if pathname can not be found via TT2's INCLUDE_PATH search
        $self->engine->service->context->template( $pathname );
        1;
    };
    warn $@ if ! $exists;
    return $exists;
}

It'd be nice to emit the warning via an app's logger too - but that's a little trickier!

@kielstr
Copy link
Author

kielstr commented Oct 11, 2017 via email

@cromedome cromedome modified the milestones: 2017-10-13, 2017-10-31 Oct 17, 2017
veryrusty added a commit that referenced this pull request Mar 28, 2018
Rework of #1354 that
  * ensures the boolean state of the return from pathname_exists is
    maintained, and
  * emits the message about a template not existing via the app's
    log engine at debug level.
@veryrusty
Copy link
Member

Merged a variant of this into master via commit e8a9f0c. This implements the above suggestion to ensure the (boolean) nature of the return is maintained, and logs the missing template (at debug level) via the app's logger.

@veryrusty veryrusty closed this Mar 28, 2018
cromedome added a commit that referenced this pull request Apr 10, 2018
    [ BUG FIXES ]
    * GH #1304: Fix the order by which config files are loaded, independently
      of their filename extension (Alberto Simões, Russell @veryrusty Jenkins)
    * GH #1400: Fix infinite recursion with exceptions that use circular
      references. (Andre Walker)
    * GH #1430: Fix `dancer2 gen` from source directory when Dancer2 not
      installed. (Tina @perlpunk Müller - Tina)
    * GH #1434: Add `validate_id` method to verify a session id before
      requesting the session engine fetch it from its data store.
      (Russell @veryrusty Jenkins)
    * GH #1435, #1438: Allow XS crush_cookie methods to return an arrayref
      of values. (Russell @veryrusty Jenkins)
    * GH #1090, #1406: Replace HTTP::Body with HTTP::Entity::Parser in
      Dancer2::Core::Request. (Russell @veryrusty Jenkins)
    * GH #1443: Update copyright year (Joseph Frazer)
    * GH #1445: Use latest HTTP::Headers::Fast (Russell @veryrusty Jenkins)

    [ ENHANCEMENTS ]
    * GH #1432: Support Content-Disposition of inline in
      send_file() (Dave Webb)
    * PR #1433: Verbose testing in AppVeyor (Graham Knop)
    * PR #1354: TemplateToolkit template engine will log (at debug level)
      if a template is not found. (Kiel R Stirling, Russell @veryrusty Jenkins)

    [ DOCUMENTATION ]
    * GH #1317: Document serializer configuration (sdeseille)
    * PR #1426: Move performance improvement information from Migration guide
      to Deployment (Pedro Melo)
cromedome added a commit that referenced this pull request Apr 20, 2018
    [ BUG FIXES ]
    * GH #1090, #1406: Replace HTTP::Body with HTTP::Entity::Parser in
      Dancer2::Core::Request. (Russell @veryrusty Jenkins)
    * GH #1292: Fix multiple attribute definitions within Plugins
      (Nigel Gregoire)
    * GH #1304: Fix the order by which config files are loaded, independently
      of their filename extension (Alberto Simões, Russell @veryrusty Jenkins)
    * GH #1400: Fix infinite recursion with exceptions that use circular
      references. (Andre Walker)
    * GH #1430: Fix `dancer2 gen` from source directory when Dancer2 not
      installed. (Tina @perlpunk Müller - Tina)
    * GH #1434: Add `validate_id` method to verify a session id before
      requesting the session engine fetch it from its data store.
      (Russell @veryrusty Jenkins)
    * GH #1435, #1438: Allow XS crush_cookie methods to return an arrayref
      of values. (Russell @veryrusty Jenkins)
    * GH #1443: Update copyright year (Joseph Frazer)
    * GH #1445: Use latest HTTP::Headers::Fast (Russell @veryrusty Jenkins)
    * PR #1447: Fix missing build requires (Mohammad S Anwar)

    [ ENHANCEMENTS ]
    * PR #1354: TemplateToolkit template engine will log (at debug level)
      if a template is not found. (Kiel R Stirling, Russell @veryrusty Jenkins)
    * GH #1432: Support Content-Disposition of inline in
      send_file() (Dave Webb)
    * PR #1433: Verbose testing in AppVeyor (Graham Knop)

    [ DOCUMENTATION ]
    * GH #1314: Documentation tweaks (David Precious)
    * GH #1317: Document serializer configuration (sdeseille)
    * GH #1386: Add Hello World example (Gabor Szabo)
    * PR #1408: List project development resources (Steve Dondley)
    * PR #1426: Move performance improvement information from Migration guide
      to Deployment (Pedro Melo)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants