#metabrainz

Probably not by adding 'data-score' even though it is not needed.

score can be accessed through row.original.score

Spread syntax (...) can probably be used to add 'data-score' only when needed.

It's not added if it's null though? At least not currently, where we use the same code for instrument collections and those have no data-score

Oh great then :)

Oops, that was a question! No, it should not be added when it’s null.

No, it wasn't (quite) a question :D

For InstrumentList I'm doing return <Table columns={columns} data={instruments} />

But on InstrumentResults we have "result", with result.instrument and result.score

Not sure what'd be the best way to make both use the react-table definitions :)

I guess I could map the instruments from results into an instruments-only array, but what about the score?

results should be rows

accessor can be tuned to get instrument data

[listenbrainz-labs] vansika merged pull request #60 (all-changes-mapping…mbid-msid-mapping): Use mbid->msid mapping to get mbid for every listen https://github.com/metabrainz/listenbrainz-labs...

reeeo

But then we have the case where say defineNameColumn would need to know to access either "name" or "instrument.name"

(and then it would be "area.name" or whatnot)

reeeeoow

reosarevok: right, adding a parameter to defineNameColumn should work.

So we should have a parameter like isSearch passed to all columns?

Wait, no, I'd have to pass the name of the results object

ruaok: have merged #60 to all-changes-mapping, tests will now have this branch as base

Sigh, why haven't we just put the score as part of the instrument or whatnot to make our life easier yet :p

instrument?

reosarevok: not isSearch but more likely accessorPrefix, so we can use it for any table (search results, reports, collections, whatnot…)

bitmap: I would have suggested lens for the above accessorPrefix, but it has been removed months ago. Do you have any other suggestion here?

Project bookbrainz-site build #2487: failed in 2 min 42 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...

reosarevok: stupid me, no need of additional dependency, just row[accessorPrefix].name

So, would you suggest doing something like accessorPrefix ? row[accessorPrefix].name : name?

Project bookbrainz-site build #2488: passed in 5 min 49 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...

is it row.name in one case? I'd suggest defineNameColumn should accept a function which returns the name from the row

that is accessor :)

accessor can be either a string or a function in Column definition

from what I understood one is row.original.name and the other is row.original.instrument.name

one problem is that Flow won't like using a string indexer there at all but it should understand a function fine

zas: should the postgres data dir on williams be moved to a home directory? right now it's in /var/lib/docker/volumes

but it works here: https://github.com/metabrainz/musicbrainz-serve...

it is just about parameterizing accessor here: https://github.com/metabrainz/musicbrainz-serve...

yeah, it works now 'cause CoreEntityT | CollectionT both have a 'name' property

Well, not just there, but everywhere really

Everywhere that we'll need for searches

but a result object with {instrument, score} doesn't

Soooo, what's the suggestion here? :D

bitmap: not sure I expressed correctly my suggestion, it is to replace the value 'name' here with an 'accessor' argument passed to 'defineNameColumn'.

I mean, I could just pass data={a map of instruments} and scores={[entityId: string]: number}...

zas: at least mb doesn't use jenkins for tests anymore, though we do use it for building docker images

'accessor' argument can default to the string value ''name'' or be passed as either a string or a function

don't think that creates any volumes I'll check the config to make sure

mb could even start using circleci to build docker images, they have a thing to save the docker build cache in their cloud or whatever

most probably the current flow-typed def for react-table is not capturing 'accessor' as function: https://github.com/tannerlinsley/react-table/bl...

yvanzo: that makes sense to me, I'm just not sure Flow will like it defaulting to a string (but I haven't checked)

and the Cell would need customizing too since original isn't the entity anymore

thus my initial suggestion to just pass an accessorPrefix (or entityPath) but that is probably not flow-friendly.

a function that takes row.original and returns the entity should work fine for both

then the accessor would be row => getEntity(row.original).name

it could default to lodash/identity so you don't have to specify it

So how would getEntity work?

[bookbrainz-site] MonkeyDo opened pull request #333 (master…entity-merging): feat: Entity merging tool https://github.com/bookbrainz/bookbrainz-site/p...

At first I thought it'd just look whether original.(any entity name) exists, but we have artist.area and whatnot so

it'd have to be different for each search list

Oh, so you define the function and pass it

yea

to Table

dunno, I forget how it works already :)

Tsk :p

if you want you can update react-table to rc.9 in whatever pr you're working on too

Any useful changes from it?

probably just bug fixes, we're on an older beta version rn

yvanzo: we should prob set DBD::Pg to 3.10.0 in the cpanfile too, the json dumps broke without it (it has some fixes for pg 10-12)

[musicbrainz-server] reosarevok opened pull request #1326 (master…MBS-10365): WIP: MBS-10365: Convert entity lists to react-table https://github.com/metabrainz/musicbrainz-serve...

MBS-10365: Refactor display of entity list/table https://tickets.metabrainz.org/browse/MBS-10365

iliekcomputers: we never really followed up on the settings goals/deadlines convo, did we? still want to do that?

ruaok: hi! Yes! Maybe Sunday or after? I'm in SF this week and will probably be really jetlagged Saturday.

gawd, I know that feeling!

I'm off some for xmas market fattening up over the weekend, so that works out well. monday or maybe tuesday evening?

Either would work for me.

and at some point I'd love for you to review the messybrainz-labs project.

not that there is a PR, but there is a whole new project now.

Yeah, I've been meaning to get up to speed on that

I also had some questions

On whether it needs to be a whole new project

Or if it could probably stay in the messybrainz repository

not sure I have an answer yet.

With the ListenBrainz and labs decomposition, we had to duplicate some work

for starters I wanted a project that I could noodle about and test some things.

Like tests for starters

ruaok: I agree with you on doing it with a clean slate initially

luckily the testing worked out and its a real viable project. (one that no longer uses 50GB of ram either!)

I was more wondering if its worth to try to move it over to the main repo

but I'm happy to merge it into the msb repo -- it would remove duplication, you're right.

MSB or LB "main repo"?

msb

yep, agreed.

To be honest, if we were starting everything rn, I would have maybe argued for keeping EVERYTHING in a big repo

then I will not implement things like manage.py, since those will magically appear later.

LB, AB, CB all of it.

that is what google does and I see it to a degree.

I'm certain I dislike the old BB way of having 5 or so repos.

It would make brainzutils redundant, and sharing code really easy

confusing as hell.

better for security too.

Indeed

Just one place to update

well, right now that isn't fully possible is it?

AB can't be py3 yet can it?

which is scary in an of itself.

and

Migrating AB should be a small task, it's the ML libraries behind it (essentia?) that's hard

exactly.

how exactly is that invoked? if that is via the command line, then fine.

I am not sure.

if we're liking to python bindings that are 2.x... ugh.

I could covert to calling an external binary and move the web base to 3

convert

Hmm, need to get some alastairp time on this, I guess.

aww yiss. test now all pass.

To discuss the best way forward

these tests have been saving my ass in making sure things are working right.

might be goo to do that when you are in BCN.

Yes!

Also

This maybe a bad idea

But

Do you think we should evaluate merging labs into LB too

iliekcomputers: 🍺

yay on the shengen visa! that must be a liberating feeling!

I don't like having duplicated rabbitmq code or having to setup sentry etc