[PATCH] Not in codereview: newUI: Move and update the text strings file for scripts

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

[PATCH] Not in codereview: newUI: Move and update the text strings file for scripts

Brian Schweitzer
Codereview doesn't like this patch - it seems to break on

rename from root/es_text.tt
rename to root/scripts/js_text.tt

in the diff, giving a "diffviewer_filediff.dest_detail may not be NULL" error.

Anyhow, this patch is part of the newUI patches.  It moves the text strings file into the /scripts directory, and updates that file with some additional and changed strings.

Brian

_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel

newUI-part3.diff (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Not in codereview: newUI: Move and update the text strings file for scripts

Nikolai Prokoschenko
Administrator
Brian Schweitzer <[hidden email]> writes:

> Codereview doesn't like this patch - it seems to break on
> rename from root/es_text.tt
> rename to root/scripts/js_text.tt
> in the diff, giving a "diffviewer_filediff.dest_detail may not be NULL" error.

As I've written in my Review Board introduction mail, ReviewBoard
currently can't handle those "git diff -M" diffs. Renames and such have
to be handled the old way -- with a plain "git diff" which sadly produces a
lot of completely deleted and newly created files instead of a rename.

I've submitted an appropriate bug report and maybe I will have to submit
a patch to fix that behaviour.

Nikolai


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

Re: [PATCH] Not in codereview: newUI: Move and update the text strings file for scripts

Brian Schweitzer
Yes, thanks, I actually found your bug ticket there earlier today when looking for some fix for this.  :)

I guess these would have to be split out, then, from the original (bigger) patch, to go to RB.  There were 10 of them, so not too too bad.  But right now, it seems like the question as to whether all the newUI patch will go through a slow review, or just go straight to trunk, seems up in the air and a bit undecided at the moment.  If it is decided to have it all go through review, only then will I go back and try to split those 10 pieces out, and then add them to RB.   Lukas/Oliver/Robert, let me know which way you want to go with that patch, and I'll handle those 10 patches going in to RB, then, if needed.  :)

Brian

On Sat, May 2, 2009 at 5:03 PM, Nikolai Prokoschenko <[hidden email]> wrote:
Brian Schweitzer <[hidden email]> writes:

> Codereview doesn't like this patch - it seems to break on
> rename from root/es_text.tt
> rename to root/scripts/js_text.tt
> in the diff, giving a "diffviewer_filediff.dest_detail may not be NULL" error.

As I've written in my Review Board introduction mail, ReviewBoard
currently can't handle those "git diff -M" diffs. Renames and such have
to be handled the old way -- with a plain "git diff" which sadly produces a
lot of completely deleted and newly created files instead of a rename.

I've submitted an appropriate bug report and maybe I will have to submit
a patch to fix that behaviour.

Nikolai


_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel


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

Re: [PATCH] Not in codereview: newUI: Move and update the text strings file for scripts

Nikolai Prokoschenko
Administrator
Brian Schweitzer <[hidden email]> writes:

> But right now, it seems like the question as to whether
> all the newUI patch will go through a slow review, or
> just go straight to trunk, seems up in the air and a bit undecided at
> the moment.  If it is decided to have it all go through
> review, only then will I go back and try to split those 10 pieces out,
> and then add them to RB.   Lukas/Oliver/Robert, let me
> know which way you want to go with that patch, and I'll handle those
> 10 patches going in to RB, then, if needed.  :)

As you probably have seen, I'm currently going through those patches bit
by bit to learn a bit more about the codebase. My main problem with
those newUI patches is that those are not self-containing, i.e. they
directly depend on other patches. While this might be appropriate in
certain cases, we should generally avoid it.

At current point, I'd prefer those patches committed into trunk, just as
the original ES has been and go from there with bug reports.

Nikolai


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

Re: [PATCH] Not in codereview: newUI: Move and update the text strings file for scripts

Brian Schweitzer
Just a reminder; if you would prefer to review the newUI patch as a whole, rather than the piecemeal parts (ie, a version of the patch which contains all the parts, thus no missing dependancies), it was sent to this list in http://lists.musicbrainz.org/pipermail/musicbrainz-devel/2009-April/003257.html , and also can be found on github:

http://github.com/brianfreud/musicbrainz-catalyst/commit/c2993506fba53238d25d6d6dcde453f844d9961b

then moving backwards to where you can see the original ES branch merge to trunk in the graph.

Brian

On Sat, May 2, 2009 at 6:54 PM, Nikolai Prokoschenko <[hidden email]> wrote:
Brian Schweitzer <[hidden email]> writes:

> But right now, it seems like the question as to whether
> all the newUI patch will go through a slow review, or
> just go straight to trunk, seems up in the air and a bit undecided at
> the moment.  If it is decided to have it all go through
> review, only then will I go back and try to split those 10 pieces out,
> and then add them to RB.   Lukas/Oliver/Robert, let me
> know which way you want to go with that patch, and I'll handle those
> 10 patches going in to RB, then, if needed.  :)

As you probably have seen, I'm currently going through those patches bit
by bit to learn a bit more about the codebase. My main problem with
those newUI patches is that those are not self-containing, i.e. they
directly depend on other patches. While this might be appropriate in
certain cases, we should generally avoid it.

At current point, I'd prefer those patches committed into trunk, just as
the original ES has been and go from there with bug reports.

Nikolai


_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel


_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel