Review Request 124613: kdesrc-build: improve message about why a full refresh is needed

[Michael Pyne]

On Aug. 7, 2015, 1:37 a.m., David Faure wrote:
> > One other thing, the "l10nSystem" class is a source updater and a build system combined, and has a 'needsRefreshed' sub that would also need updated. Something like this diff should work:
> > 
> >     diff --git a/modules/ksb/l10nSystem.pm b/modules/ksb/l10nSystem.pm
> >     index e5756f8..0101824 100644
> >     --- a/modules/ksb/l10nSystem.pm
> >     +++ b/modules/ksb/l10nSystem.pm
> >     @@ -24,7 +24,9 @@ sub new
> >          # TODO: Support different localization branches?
> >      
> >          $module->setOption('module-base-path', 'trunk/l10n-kde4');
> >     -    return bless { module => $module, needsRefreshed => 1 }, $class;
> >     +
> >     +    my $refreshMessage = "couldn't verify the source is unchanged";
> >     +    return bless { module => $module, needsRefreshed => $refreshMessage }, $class;
> >      }
> >      
> >      sub module
> >     @@ -75,7 +77,7 @@ sub updateInternal
> >              $self->check_module_validity();
> >              my $count = $self->update_module_path(@dirs);
> >      
> >     -        $self->{needsRefreshed} = 0 if $count == 0;
> >     +        $self->{needsRefreshed} = '' if $count == 0;
> >              return $count;
> >          }
> >          else {
> >     @@ -103,7 +105,7 @@ sub needsRefreshed
> >      {
> >          my $self = shift;
> >      
> >     -    # Should be 1 except if no update happened.
> >     +    # Should be a 'reason' string except if no update happened.
> >          return $self->{needsRefreshed};
> >      }
> > 
> > With those taken care of you have a +1 from me.
> 
> David Faure wrote:
>     Since the value is set to empty (in the 2nd hunk) when no files were updated by "svn update", doesn't this mean that the message in the first hunk should be "an update happened"?

That would be a more positive (and probably more understandable) way of wording it, yes.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124613/#review83527
-----------------------------------------------------------


On Aug. 9, 2015, 9:36 a.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124613/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2015, 9:36 a.m.)
> 
> 
> Review request for Build System and Michael Pyne.
> 
> 
> Repository: kdesrc-build
> 
> 
> Description
> -------
> 
> i.e. be more precise than "meets other building criteria"
> 
> REVIEW: 124613
> 
> 
> Diffs
> -----
> 
>   modules/ksb/IPC.pm 0e55c5fe5c2a8771f6b9b159a684bdcc6dda52a7 
>   modules/ksb/Module.pm a7a7fbcc5e30cbb4c52979d87407a3250b80f2b2 
>   modules/ksb/l10nSystem.pm e5756f85026ad9b95533d2989746ab91f87920a6 
>   modules/ksb/BuildSystem.pm 24963040d77dc93f8f86a2f19755c617713bbd8a 
> 
> Diff: https://git.reviewboard.kde.org/r/124613/diff/
> 
> 
> Testing
> -------
> 
> Example:
> 
> $ kdesrc-build --refresh plasma-nm
> 
> Building plasma-nm from kf5-network-management (1/1)
>         Updating plasma-nm (to branch master)
>         No source update, but the option refresh-build was set
>         Source update complete for plasma-nm: no files affected
>         Preparing build system for plasma-nm.
> ...
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20150809/8f399543/attachment.html>