Closed arahayrabedian closed 6 years ago
Hey man! Thanks for all the work you've put into this. I gave a quick look and it looks like you did a lot of work here!
Give me a week to go through this, as I'll probably only be able to get to it by next weekend. I should have some comments by next Saturday.
it was a little more involved than I was expecting to be honest, if you see a way to simplify it i'm all ears.
take your time on checking it out, I'm probably still gonna throw in minor modifications.
@arahayrabedian I'll admit I don't gork this completely as it's a large change. But it looks good on the surface. I assume you've tested it on the older config format to make sure it works in a backwards compatible method.
There's just one comment I added. Once you've looked at that, we can go ahead and merge this. I'll probably go through the code afterwards – if I ever find the time!
Looks good overall. Thanks for all the work you've put into this. Let's get this released!
@theonejb yeah it's mostly plumbing, but I think it'd be a good idea for you to understand what you pull in to your project, i'm happy to wait (i can run this experimentally from a branch until you're comfortable with it)
It doesn't have anything to be backward compatible per-se, it does not make any changes to the config file, it only looks for a ordering.yaml
file in the bucket, attempting to parse/use it if it's found.
one issue could be users being presented with a blank image in their album if they add in a ordering.yaml
file prior to upgrading, i think that's about it (and is easily avoidable revertible by dropping the file from the bucket)
if there's any particular concerns i'm more than happy to clarify them
sweet, you're going to love what i have in store in 2 minutes :) i promise it's quick
It works, ~it still needs a final passthrough/review on my part~, but just for final-state-ish awareness I opened the PR. ~I still need to look at the PR itself,~ I'm sure there are glaring mistakes.
How it works at a very very high level:
AlbumOrderingConfig
objectAlbumOrderingConfig
in order to create anAlbumOrdering
object, which gets passed around with the actual ordering of an album. ThisAlbumOrdering
is a definitive definition of an Album's ordering, and is used both with and without an ordering.yml file being presentFailure modes that are handled (barring bugs or oversights):
We also follow some ground rules (commented in the code in the relevant section):
* i say silently, but there is a message printed.
Things to do:
AlbumOrdering
object itself, as this alone has a bit of a cost to generate (though no network costs)Let me know what you think.