Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For #35869: do version token replacement in the args setting #30

Merged
merged 4 commits into from
Apr 8, 2016

Conversation

kporangehat
Copy link

Apply our version token replacement to the *_args setting like we currently do for the path and icon settings. This enables clients who use a wrapper to launch their DCC, take advantage of the version tokens in the *_args setting. And allows them to support launching multiple versions of their DCC using the versions setting.

KP added 2 commits April 5, 2016 10:50
Apply our version token replacement to the *_args setting like we currently do for the path and icon settings. This enables clients who use a wrapper to launch their DCC, take advantage of the version tokens in the *_args setting. And allows them to support launching multiple versions of their DCC using the `versions` setting.
@kporangehat
Copy link
Author

Hmmm. I see #25 now and think that's maybe a nicer implementation. thoughts?

@robblau
Copy link
Contributor

robblau commented Apr 5, 2016

First thought is that in #25 there is logic about falling back to the first version configured if the arg is passed in as None. Doing that only for the args as opposed to the other places where version is used seems inconsistent.

An approach like this makes more sense to me with the current implementation and we should talk through adding in smarter default behavior. I'm not sure what workflow ends up with version being passed in as None if versions have been configured.

Lets go forward with this kind of implementation and I'll ask in the other PR.

@robblau
Copy link
Contributor

robblau commented Apr 5, 2016

Ok... looking deeper, I'm seeing the same code for _get_app_path (using versions[0]).
Les use #25 instead, although a refactor to centralize that logic would be awesome.

@kporangehat
Copy link
Author

Okay I commented on #25 with the refactor suggestion and another minor comment. Will await movement on that one.

@kporangehat kporangehat merged commit cea7316 into master Apr 8, 2016
@manneohrstrom manneohrstrom deleted the 35869_version_in_args branch January 21, 2017 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants