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"
2010-03-25 08420, 2010
luks
perhaps you think too much?
2010-03-25 08430, 2010
luks
it's an overview for beginners
2010-03-25 08453, 2010
brianfreud
*shrug*
2010-03-25 08440, 2010
brianfreud
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.
2010-03-25 08454, 2010
brianfreud sleeps
2010-03-25 08402, 2010
luks
there are many other things stored in the database
2010-03-25 08424, 2010
luks
ISRCs, ISWCs, CD TOCs
2010-03-25 08449, 2010
brianfreud
yes, but not ones which are both an entity (aka 'object') and which are exposed to the user
2010-03-25 08404, 2010
brianfreud
unless there's a ISRC entity, ISWC entity, etc now?
URL can be linked to anything, ISRC can be linked only to recordings
2010-03-25 08431, 2010
luks
it's not that big difference
2010-03-25 08428, 2010
ijabz joined the channel
2010-03-25 08418, 2010
harshit_ved joined the channel
2010-03-25 08441, 2010
ijabz joined the channel
2010-03-25 08423, 2010
Leftmost joined the channel
2010-03-25 08431, 2010
djce joined the channel
2010-03-25 08438, 2010
aCiD2
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
2010-03-25 08459, 2010
jensl
the file suggests itself to connect to as a host :D
2010-03-25 08432, 2010
aCiD2
woohoo, MOD_ADD_RELEASE is now upgraded to Edit::Release::Create :)
2010-03-25 08424, 2010
warp
aCiD2 \o/
2010-03-25 08433, 2010
avi| joined the channel
2010-03-25 08437, 2010
avi| has left the channel
2010-03-25 08409, 2010
warp
aCiD2: I'm wondering about this:
2010-03-25 08413, 2010
warp
Can't locate object method "log" via package "MusicBrainz::Server::Context" at /home/warp/code/mb/lib/MusicBrainz/Server/Data/Search.pm
2010-03-25 08428, 2010
warp
aCiD2: should a log method be added to that context?
2010-03-25 08400, 2010
aCiD2
i couldn't decide on that
2010-03-25 08414, 2010
aCiD2
possibly, so scripts can make use of a logger
2010-03-25 08428, 2010
warp
Why is it only a problem in the tests? Is that context only used for testing?
2010-03-25 08406, 2010
aCiD2
the tests don't start up a catalyst stack, and that's where the logger is. iirc
2010-03-25 08438, 2010
warp
how does the MusicBrainz::Server::Context acquire a test method in that scenario?
2010-03-25 08438, 2010
aCiD2
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)
2010-03-25 08444, 2010
aCiD2
a test method?
2010-03-25 08458, 2010
warp
a log method, sorry.
2010-03-25 08433, 2010
aCiD2
it doesn't :) It would get one by adding "has 'logger' => ( handles => [qw( log )] )" to MusicBrainz::Server::Context.
2010-03-25 08448, 2010
aCiD2
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?
in fact, that whole function smells by the fact that $c is passed in
2010-03-25 08458, 2010
aCiD2
it should be using $self->c
2010-03-25 08432, 2010
warp
apparantly the function is only used in the tests. I've never seen this happen when running script/musicbrainz_server.pl.
2010-03-25 08414, 2010
aCiD2
Because if you look at Controller::Search it passes in $c, which is a MusicBrainz::Server, not a MusicBrainz::Server::Context
2010-03-25 08437, 2010
aCiD2
(line 159 of Controller/Search.pm)
2010-03-25 08405, 2010
warp
right
2010-03-25 08419, 2010
warp
so production code is using that function, but is using it differently from test code.
2010-03-25 08429, 2010
aCiD2
yea
2010-03-25 08456, 2010
warp
still, most tests seem to use MusicBrainz::Server::Context as if it is catalyst.
2010-03-25 08429, 2010
aCiD2
what do you mean?
2010-03-25 08439, 2010
warp
$c->model() and such.
2010-03-25 08400, 2010
aCiD2
sure, because MusicBrainz::Server::Context provides that method
2010-03-25 08423, 2010
warp
but normally MusicBrainz::Server::Context isn't the one providing that method, right? or did I misunderstand that part.
2010-03-25 08431, 2010
aCiD2
it normally is
2010-03-25 08443, 2010
aCiD2
See MusicBrainz::Server::Model::MB for where the context and all models are created
2010-03-25 08456, 2010
warp
from what I can see, $c is normally a MusicBrainz::Server. not a MusicBrainz::Server::Context.
2010-03-25 08454, 2010
aCiD2
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)
2010-03-25 08456, 2010
jensl joined the channel
2010-03-25 08440, 2010
warp
warn "c is a MusicBrainz::Server" if $c->isa ('MusicBrainz::Server');
2010-03-25 08443, 2010
warp
warn "c is not a MusicBrainz::Server::Context" unless $c->isa ('MusicBrainz::Server::Context');
2010-03-25 08446, 2010
warp
c is a MusicBrainz::Server at /home/warp/code/mb/script/../lib/MusicBrainz/Server/Controller/User.pm line 526.
2010-03-25 08449, 2010
warp
c is a MusicBrainz::Server at /home/warp/code/mb/script/../lib/MusicBrainz/Server/Controller/User.pm line 527.
2010-03-25 08452, 2010
warp
c is not a MusicBrainz::Server::Context at /home/warp/code/mb/script/../lib/MusicBrainz/Server/Controller/User.pm line 528.
2010-03-25 08406, 2010
warp
aCiD2: that $c is not a Context. are we talking about different things?
2010-03-25 08407, 2010
aCiD2
where are you warning? $c is a MusicBrainz::Server in controllers
2010-03-25 08410, 2010
aCiD2
but not in data objects
2010-03-25 08414, 2010
warp
ah!
2010-03-25 08428, 2010
aCiD2
a data object should not know that it is being ran in catalyst or a script or a test
2010-03-25 08416, 2010
warp
so that's why external_search gets the $c passed in, it's an entirely different class.
2010-03-25 08431, 2010
aCiD2
yep
2010-03-25 08436, 2010
aCiD2
but it shouldn't get it passed in
2010-03-25 08443, 2010
aCiD2
it should be able to use $self->context
2010-03-25 08443, 2010
warp nods.
2010-03-25 08449, 2010
aCiD2
(or $self->c, I forget the name)
2010-03-25 08406, 2010
warp
anyway, it's just one log line, I'll remove it :)
2010-03-25 08420, 2010
aCiD2
yea, it looks like debug cruft :)
2010-03-25 08408, 2010
warp
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.
2010-03-25 08400, 2010
aCiD2
+100000 to that
2010-03-25 08423, 2010
aCiD2
It sure would be nice to stop having a broken master :)
2010-03-25 08407, 2010
ruaok joined the channel
2010-03-25 08410, 2010
aCiD2
ruaok: email plox!
2010-03-25 08421, 2010
warp
aCiD2: I remember you were working on coverart stuff a while ago, is that in master?
If you need it though, you could make a temporary integration branch and merge your work and it
2010-03-25 08449, 2010
warp
yes, or I take some more effort to understand your code so I can review it.
2010-03-25 08423, 2010
ruaok
moin.
2010-03-25 08432, 2010
ruaok
one of my first priorities today is review stuff.
2010-03-25 08443, 2010
ruaok
should I start with 593 after the email aCiD2 ?
2010-03-25 08452, 2010
aCiD2
I don't mind, none of the reviews are pressing for me atm
2010-03-25 08406, 2010
aCiD2
but the email is quite important, we might want to discuss it here once you've read it with warp
2010-03-25 08417, 2010
ruaok nods
2010-03-25 08403, 2010
warp
hm, email?
2010-03-25 08420, 2010
aCiD2
i forgot to cc :(
2010-03-25 08431, 2010
ruaok
a copy is on the way warp
2010-03-25 08444, 2010
ruaok
aCiD2: we're talking about edit rows ids, right?
2010-03-25 08451, 2010
warp
ah
2010-03-25 08455, 2010
ruaok
since edits have no MBIDs.
2010-03-25 08455, 2010
aCiD2
yea
2010-03-25 08412, 2010
ruaok
I'm wondering how important it is to keep those the same.
2010-03-25 08420, 2010
ruaok
clearly all the links to old edits would break.
2010-03-25 08424, 2010
aCiD2
well, we have a lot of edit notes that link to other edits
2010-03-25 08427, 2010
ruaok
but, is that a big deal?
2010-03-25 08433, 2010
warp
ah, ouch.
2010-03-25 08435, 2010
aCiD2
They wouldn't break
2010-03-25 08441, 2010
ruaok
those need to be preserved, of course.
2010-03-25 08458, 2010
aCiD2
basically warp
2010-03-25 08458, 2010
ruaok
as long as we're internally consistent we should be fine, IMHO.
2010-03-25 08407, 2010
warp
aCiD2: yeah, I just read the mail.
2010-03-25 08414, 2010
aCiD2
oh, you got it now, cool :)
2010-03-25 08441, 2010
warp
we need more bloat, I say we add a redirect table. ;)
2010-03-25 08446, 2010
aCiD2
hehe
2010-03-25 08418, 2010
aCiD2
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"
2010-03-25 08426, 2010
warp
ruaok: internally consistent in the sense that we go about rewriting links in edit notes in the migration step?
2010-03-25 08426, 2010
aCiD2
the other option, is we duplicate edit notes over the migrated edits too
2010-03-25 08439, 2010
ruaok
warp: yes
2010-03-25 08403, 2010
ruaok
aCiD2: lets step back for a minute and define our landscape.
2010-03-25 08408, 2010
aCiD2
ok
2010-03-25 08418, 2010
ruaok
we clearly need to be internally consistent.
2010-03-25 08424, 2010
ruaok
every edit note needs to be fixed up.
2010-03-25 08446, 2010
ruaok
so warp is rifght we need to redirect table, but it can be temporary during the upgrade.
2010-03-25 08401, 2010
ruaok
so, internal consistency is not hard, is it?
2010-03-25 08402, 2010
aCiD2
I'm not sure we need to rewrite any edit notes
2010-03-25 08418, 2010
aCiD2
here's an example though, just to make sure we're all on the same page
2010-03-25 08421, 2010
ruaok
if the edit ids change, then yes.
2010-03-25 08427, 2010
aCiD2
they aren't changing, let me explain
2010-03-25 08444, 2010
aCiD2
lets take edit #123 which is a pre-ngs add release edit type. It adds a release with 3 release events
2010-03-25 08411, 2010
aCiD2
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
2010-03-25 08412, 2010
warp
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.
2010-03-25 08444, 2010
aCiD2
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
2010-03-25 08403, 2010
ruaok
how do you figure out what 900 is?
2010-03-25 08410, 2010
aCiD2
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
2010-03-25 08413, 2010
ruaok
select max(id) from edit_closed ?
2010-03-25 08430, 2010
aCiD2
ruaok: pretty much, but I don't do it myself, I just let postgresql do it (the sequence is reset before migration begins)
2010-03-25 08433, 2010
warp
ALTER SEQUENCE foo RESTART 900; ?
2010-03-25 08444, 2010
aCiD2
ALTER SEQUENCE edit_id_seq RESTART max_id(public.moderation_closed)
2010-03-25 08402, 2010
aCiD2
so anything we insert without an ID by default goes after ever edit
2010-03-25 08416, 2010
aCiD2
unless we specify an id, which means we avoid the sequence
2010-03-25 08419, 2010
ruaok
ok, seems fine to me.
2010-03-25 08425, 2010
ruaok
is there a question left then? ;)
2010-03-25 08433, 2010
aCiD2
yes, which one do you want me to do :)
2010-03-25 08436, 2010
aCiD2
i proposed 2 solutions
2010-03-25 08440, 2010
warp
aCiD2: I'd probably have modbot add notes on both sides of the link.
