search

indiehd / web-api

GNU Affero General Public License v3.0
6 stars 4 forks source link

refactor: add casts to models #164

Closed mblarsen closed 4 years ago

mblarsen commented 4 years ago

When review please check that I've created the correct decimal casts.

decimal:X where X is the number of digits.

I'm not sure if that means fractional digits. E.g. decimal(15, 2) does that mean X = 15 or X = 2?.

Do we have any tests that works with models that has decimal types like album or flac file @cbj4074 @poppabear8883 already or should I add a new test to test this?

Tracked it down to this:

    protected function asDecimal($value, $decimals)
    {
        return number_format($value, $decimals, '.', '');
    }

So it means digits = fractional digits.

cbj4074 commented 4 years ago

@mblarsen Thanks for digging into the decimal behavior.

At the moment, I don't believe that we test specifically for decimal precision anywhere. So, yes, it would be very helpful to add tests to that effect.

cbj4074 commented 4 years ago

@mblarsen I'm going to hold-off on merging this, in case you intended to add tests as part of this PR.

If you'd rather just merge it and open a new PR for tests, that's fine, too! You can feel free to merge yourself, too, given that you have an approved review now.

mblarsen commented 4 years ago

@cbj4074 re: casts. Values read from the database are strings so I basically selected all non string columns from the migrations and added as casts.

Except keys. As you shouldn't bother too much about them, meaning they shouldn't really be exposed in a way that they are used as actual values. Sure you might read it and set as fk somewhere, but what the actual value is doesn't matter. Unlike money for instance where you may add or subtract.

Re: test, yes I wlll add some. Maybe to the end point tests. I mean the casts themselves shouldn't be tested, that is what Laravel does, but we could write some tests regarding money or compression ratio to ensure we are using decimal correctly.

Regarding that. I realize that I'm not. The decimal:x casts using number_format. That means it outputs a string. This isn't really what we want I think.

I'll rethink what this cast PR with regards to database decimals. On the top of my head:

The difference is on the importance of precision. Where in the money case we don't want rounding errors, but really we don't case if compression ratio is 0.5849428348024810342 or 0.5849428348024810343.

Another example to consider is frequency. They are currently decimals in the DB, but really they are not used in computation, so they are really strings. For that decimal:x is fine. But really it could just be stored as a string to begin with.

mblarsen commented 4 years ago

On album the bitreate is set to 15,7 example value 12345678.1234567 (8 + 7 = 15) but are they not always things like 128kbps 320kbps, etc? That would make the type a flat int.

The play_time_seconds also has 7 fractional digits (11,7) but do you have sub second values?

Similarly on Song the preview start is 7,3 again sub second values?

If yes to the seconds then perhaps we should make them she same precision? E.g. 11,3

Any thoughts @cbj4074

mblarsen commented 4 years ago

I've now changed the implementation to my notes in https://github.com/indiehd/web-api/pull/164#issuecomment-641668179.

Money is now used on Album and Song. I had some issues getting the tests to work. Basically if an album includes a price (in the test) the expected value that you pass to getJsonStructure needs to have its final price set before hand. For some reason et serializes to '1' instead of e.g. '999' when there is a price. Not sure why.

I should find out why, but this only relates to tests. So not super high importance.

mblarsen commented 4 years ago

I'd like to update the casts based on your responses to https://github.com/indiehd/web-api/pull/164#issuecomment-641689542 before we merge. Given than other things are okay of course :)

cbj4074 commented 4 years ago

These are all excellent points, and I concur on all counts.

And it makes sense to store audio frequency as a string for the reason you mentioned (feel free to make that change in this PR, if you like).

cbj4074 commented 4 years ago

On album the bitreate is set to 15,7 example value 12345678.1234567 (8 + 7 = 15) but are they not always things like 128kbps 320kbps, etc? That would make the type a flat int.

The audio decoding tools that we use do indeed observe a scale of 7, as in your example. In fact, of all the songs in the audio catalog currently, none of them are nice, round integers like 128 or 320. The range of values is roughly 50000 to 250000, and all have a scale of 7. I'm not entirely sure what the unit is here, either, but bits per second seems likely.

We use this information only for validation, to ensure that the audio file is not damaged or encoded incorrectly upon upload. One could argue that this value is therefore pointless to store, but I'm a fan of "nerd stats", and we do show this value in the audio file details modal.

The play_time_seconds also has 7 fractional digits (11,7) but do you have sub second values?

Yes, the decoding tools do use 7 digits of scale, and I didn't see any reason to truncate the values.

Similarly on Song the preview start is 7,3 again sub second values?

Yes indeed, although, this one is somewhat arbitrary in that it is under our control, i.e., this is a value that we specify (vs. some "read-only" value that comes from another tool). That said, the encoding tools that we use to generate the audio preview files gladly accept at least 3 digits of scale (and likely many more). Whether or not any artist is actually going to specify 3 digits of scale when adjusting his audio preview(s) is another matter, but, why not allow for it?

If yes to the seconds then perhaps we should make them she same precision? E.g. 11,3

I'm not sure that this would benefit us, necessarily, given my explanation, above. Sure, it might be "simpler", but the two values are not distinctly related.

ETA: Just to clarify on this point, the artist specifies how far into the track the audio preview should begin, with up to 3 digits of scale, we store that value, and then we generate the audio file from it. After we generate that file, we don't store the play_time_seconds for the preview file (we only store it for the original, lossless file that the artist uploaded).