search

getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
218 stars 48 forks source link

Fix #358 #359

Closed BigRoy closed 5 years ago

BigRoy commented 5 years ago

This avoids get_application directly merging the environment but solely parses the .toml for #358. Whether this is the best solution I'm not too sure, but in my case this is exactly functional for our produtions and fixes the issue.

I've implemented in the way I described in the issue:

My proposal would be to change: avalon.lib.get_application to only read out the toml, but not perform the environment management and leave that up to the action. We will need to read the .toml at least before "running" it to load the icon and color to be displayed in the Launcher

So visually it functions the same, it just now merges the environment at the end upon the call to environ() of the Application class.

davidlatwe commented 5 years ago

Can confirm this fix the placeholder bug :)

But I found this, not sure what this module is for, however it looks like there needs an update due to the removed arg environment ?

https://github.com/getavalon/core/blob/87d89285f6f5062216c9e7187070482389e4911c/avalon/session.py#L107-L111

BigRoy commented 5 years ago

Ah, thanks. Well spotted - will look into that and see what it is for. :)

BigRoy commented 5 years ago

Thanks, will still fix that thing @davidlatwe mentioned and then get ready to merge this.

BigRoy commented 5 years ago

I changed the call in session.py - I believe currently that module doesn't do anything and at the time was a draft implemented by @mottosso as an idea of turning the current session into a class. Yet it was never thoroughly tested nor used I think.

Maybe we should drop it at some point (to remove the dead code) or implement it if it has a purpose in the foreseeable future.

This PR should be ready to merge. :)