#musicbrainz-devel

everybody knows what URL is

:P

I s*objects*entities, then interpreted "does not fully cover all the objects in the database" as "all entities in the database are mentioned, though some in not very much detail"

perhaps you think too much?

it's an overview for beginners

*shrug*

everyone knows what a URL is; not everyone, esp a newbie, knows what object types are in the db. Hence my expecting to see URL mentioned, if only so the newbie knows they are actually stored in the db.

there are many other things stored in the database

ISRCs, ISWCs, CD TOCs

yes, but not ones which are both an entity (aka 'object') and which are exposed to the user

unless there's a ISRC entity, ISWC entity, etc now?

does it matter?

ISRC's don't have their own page; URLs do

does it matter? :)

http://test.musicbrainz.org/url/97e9356f-dd75-4... and so on :D

URL can be linked to anything, ISRC can be linked only to recordings

it's not that big difference

I love it when emacs tries to be clever. I hit C-x C-f to open a file and "edit.display_data.old.name" is under the cursor and it assumes I want to connect to that as a host

the file suggests itself to connect to as a host :D

woohoo, MOD_ADD_RELEASE is now upgraded to Edit::Release::Create :)

aCiD2 \o/

aCiD2: I'm wondering about this:

Can't locate object method "log" via package "MusicBrainz::Server::Context" at /home/warp/code/mb/lib/MusicBrainz/Server/Data/Search.pm

aCiD2: should a log method be added to that context?

i couldn't decide on that

possibly, so scripts can make use of a logger

Why is it only a problem in the tests? Is that context only used for testing?

the tests don't start up a catalyst stack, and that's where the logger is. iirc

how does the MusicBrainz::Server::Context acquire a test method in that scenario?

I actually think somewhere there's a $c->log added by ruaok or someone who didn't understand that that's not actually a Catalyst $c (always)

a test method?

a log method, sorry.

it doesn't :) It would get one by adding "has 'logger' => ( handles => [qw( log )] )" to MusicBrainz::Server::Context.

But I think the real problem is the line that assumes there is a logger that just shouldn't be there - what line is that?

417

in what file?

/home/warp/code/mb/lib/MusicBrainz/Server/Data/Search.pm

ta

yea, that shouldn't be there

in fact, that whole function smells by the fact that $c is passed in

it should be using $self->c

apparantly the function is only used in the tests. I've never seen this happen when running script/musicbrainz_server.pl.

Because if you look at Controller::Search it passes in $c, which is a MusicBrainz::Server, not a MusicBrainz::Server::Context

(line 159 of Controller/Search.pm)

right

so production code is using that function, but is using it differently from test code.

yea

still, most tests seem to use MusicBrainz::Server::Context as if it is catalyst.

what do you mean?

$c->model() and such.

sure, because MusicBrainz::Server::Context provides that method

but normally MusicBrainz::Server::Context isn't the one providing that method, right? or did I misunderstand that part.

it normally is

See MusicBrainz::Server::Model::MB for where the context and all models are created

from what I can see, $c is normally a MusicBrainz::Server. not a MusicBrainz::Server::Context.

nope, it's a MusicBrainz::Server::Context. MB.pm:73 is where Data objects are created, and notice they are being passed $self->context. context is a lazy attribute in MB, but it gets constructed as Context->new on line 25 (in a production environment, it'll be a test context in a test environment, as line 21 shows)

warn "c is a MusicBrainz::Server" if $c->isa ('MusicBrainz::Server');

c is a MusicBrainz::Server at /home/warp/code/mb/script/../lib/MusicBrainz/Server/Controller/User.pm line 526.

c is a MusicBrainz::Server at /home/warp/code/mb/script/../lib/MusicBrainz/Server/Controller/User.pm line 527.

c is not a MusicBrainz::Server::Context at /home/warp/code/mb/script/../lib/MusicBrainz/Server/Controller/User.pm line 528.

aCiD2: that $c is not a Context. are we talking about different things?

where are you warning? $c is a MusicBrainz::Server in controllers

but not in data objects

ah!

a data object should not know that it is being ran in catalyst or a script or a test

so that's why external_search gets the $c passed in, it's an entirely different class.

yep

but it shouldn't get it passed in

it should be able to use $self->context

(or $self->c, I forget the name)

anyway, it's just one log line, I'll remove it :)

yea, it looks like debug cruft :)

aCiD2: i'd like to get most tests fixed, and those that aren't easily fixable disabled. so that the whole thing passes, and I can start running it regularly and know when I broke something.

+100000 to that

It sure would be nice to stop having a broken master :)

ruaok: email plox!

aCiD2: I remember you were working on coverart stuff a while ago, is that in master?

it will be as soon as someone gives me a ship it for http://codereview.musicbrainz.org/r/593

ah :)

If you need it though, you could make a temporary integration branch and merge your work and it

yes, or I take some more effort to understand your code so I can review it.

moin.

one of my first priorities today is review stuff.

should I start with 593 after the email aCiD2 ?

I don't mind, none of the reviews are pressing for me atm

but the email is quite important, we might want to discuss it here once you've read it with warp

hm, email?

i forgot to cc :(

a copy is on the way warp

aCiD2: we're talking about edit rows ids, right?

ah

since edits have no MBIDs.

yea

I'm wondering how important it is to keep those the same.

clearly all the links to old edits would break.

well, we have a lot of edit notes that link to other edits

but, is that a big deal?

ah, ouch.

They wouldn't break

those need to be preserved, of course.

basically warp

as long as we're internally consistent we should be fine, IMHO.

aCiD2: yeah, I just read the mail.

oh, you got it now, cool :)

we need more bloat, I say we add a redirect table. ;)

hehe

I'm thinking that we use the edit id for the first migrated edit, and autogenerate a new id for the other ones. Then ModBot adds an edit note saying "HEY! Check out #original for any possible edit notes"

ruaok: internally consistent in the sense that we go about rewriting links in edit notes in the migration step?

the other option, is we duplicate edit notes over the migrated edits too

warp: yes

aCiD2: lets step back for a minute and define our landscape.

ok

we clearly need to be internally consistent.

every edit note needs to be fixed up.

so warp is rifght we need to redirect table, but it can be temporary during the upgrade.

so, internal consistency is not hard, is it?

I'm not sure we need to rewrite any edit notes

here's an example though, just to make sure we're all on the same page

if the edit ids change, then yes.

they aren't changing, let me explain

lets take edit #123 which is a pre-ngs add release edit type. It adds a release with 3 release events

during migration, edit #123 will become an add release event for *1* of these release events. EDits #900 and #901 will be created for the 2 other events

ruaok: aCiD2 is claiming they won't change. some of them explode and become more than one. but those IDs can be assigned to new ids which are not in use on the current server.

so anything that links to edit #123 still links to the correct edit, unless they are specifically talking about release events, in which case it could be the details are actually in #900 or #901

how do you figure out what 900 is?

hence my suggestions that we either link edits #900 and #901 to #123 via modbot, or duplicate all edit notes over #900 and #901 too

select max(id) from edit_closed ?

ruaok: pretty much, but I don't do it myself, I just let postgresql do it (the sequence is reset before migration begins)

ALTER SEQUENCE foo RESTART 900; ?

ALTER SEQUENCE edit_id_seq RESTART max_id(public.moderation_closed)

so anything we insert without an ID by default goes after ever edit

unless we specify an id, which means we avoid the sequence

ok, seems fine to me.

is there a question left then? ;)

yes, which one do you want me to do :)

i proposed 2 solutions

aCiD2: I'd probably have modbot add notes on both sides of the link.