Re: Please critique this script -- Read emails and write URLs into bookmarks

Replies are listed 'Best First'.
Re^2: Please critique this script -- Read emails and write URLs into bookmarks by Anonymous Monk on Oct 06, 2016 at 18:59 UTC
There is only one valid URL per message. This is a situational constraint (also written in the spec). I know that because these are 'Emails-to-self' that contain at max two URLs (one in the plain text part, one in the HTML part if it exists) Maybe it's a bit difficult to read but the regex is transforming the subject into a suitable filename, escaping problematic characters. I tried to be sensible about the error checking and assumed the modules do error handling for their part, outside of that there's hardly any code in its own right. Now that I think of it though, input should have been checked. That was a mistake. What else do you think needs explicit error handling?	[reply]
Re^3: Please critique this script -- Read emails and write URLs into bookmarks by talexb (Chancellor) on Oct 06, 2016 at 19:06 UTC
I would check the following: Input arguments -- if they are files, do they exist, and are they readable and the right type? Object from Path::Tiny -- check that this is valid. Perhaps check after the call to `URI::Find->new` so see that there is something in the `@uris` list? To me, that's basic defensive programming. Check everything -- you never know when Something Weird will happen; it's better that your script captures it rather than the script dying for some unknown reason. Alex / talexb / Toronto Thanks PJ. We owe you so much. Groklaw -- RIP -- 2003 to 2013.	[reply] [d/l] [select]
Re^4: Please critique this script -- Read emails and write URLs into bookmarks by Anonymous Monk on Oct 07, 2016 at 08:13 UTC
I light of my brainfart yesterday I felt compelled to show a revised script including proper error handling: I blame it on the meds! #!/usr/bin/perl # read emails I sent myself containing just a link # and make a HTML-redirect based bookmark out of them # # SPEC # read given files as internet messages # use subject as title and filename, escape as needed # use one URL in body as href (if there a two they are both the same) # and output one HTML file for each bookmark use strict; use warnings; use Carp qw(cluck confess); use Encode; use Email::Simple; use HTML::Entities; use Path::Tiny; use URI::Find; use version; our $VERSION = qv('0.0.1'); if ( @ARGV == 0 ) { confess 'Invalid number of arguments' } foreach my $in_filepath (@ARGV) { if ( not path($in_filepath)->exists ) { cluck "$in_filepath: Input file '$in_filepath' does not exist" +; next; } my $in_file = Path::Tiny->new($in_filepath)->slurp or confess "$in_filepath: Fatal error in module"; my $message = Email::Simple->new($in_file) or confess "$in_filepath: Fatal error in module"; if ( not length $message->as_string or $message->as_string =~ /\A\s\z/ ) { cluck "$in_filepath: Invalid message"; next; } if ( not length $message->header('Subject') ) { cluck "$in_filepath: Invalid subject"; next; } my $subject = decode( 'MIME-Header', $message->header('Subject') ) or confess "$in_filepath: Fatal error in module"; my $title = HTML::Entities::encode($subject) or confess "$in_filepath: Fatal error in module"; my $out_filepath = $subject =~ s/[%\/\?<>\\:\\\|":]/_/gr . '.URL.H +TML'; # TODO IMPR URL::Search might be nicer than URI::Find for some cas +es my @uris; my $finder = URI::Find->new( sub { push @uris, $_[0] } ); $finder->find( \$message->body ); if ( @uris == 0 ) { cluck "$in_filepath: Invalid number of URIs"; +next; } if ( path($out_filepath)->exists ) { cluck "$in_filepath: Output file '$out_filepath' exists"; next; } path($out_filepath)->spew( "<!DOCTYPE html><title>$title</title><meta http-equiv=\"refresh\" cont +ent=\"0; url=@uris\">", ) or confess "$in_filepath: Fatal error in module"; } [download]	[reply] [d/l]
Re^5: Please critique this script -- Read emails and write URLs into bookmarks by talexb (Chancellor) on Oct 07, 2016 at 20:47 UTC
Re^4: Please critique this script -- Read emails and write URLs into bookmarks by Anonymous Monk on Oct 06, 2016 at 19:28 UTC
I forgot to check `URI::Find->new` as well. Thanks for that. About the others, maybe I should elaborate my ideology here: I usually prefer technical error messages to 'clean' ones. Especially when I'm not perfectly sure where the errors could come from or if I don't have the time to check all odds and ends. The rationale being that my own error messages could only be worse. The errors are handled. They produce descriptive (if not overly verbose) messages. But in a script of this nature developer and user are usually quite close. So this verbosity turns out to be very useful. However, checking the number of input arguments and checking if $uris is not empty are clearly omissions which I will fix.	[reply] [d/l]
Re^5: Please critique this script -- Read emails and write URLs into bookmarks by Anonymous Monk on Oct 06, 2016 at 19:49 UTC
Re^6: Please critique this script -- Read emails and write URLs into bookmarks by Anonymous Monk on Oct 07, 2016 at 05:25 UTC


Welcome to the Monastery
	PerlMonks