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

Replace {version} in launch args #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skral
Copy link

@skral skral commented Nov 10, 2015

It makes sense to replace the {version} tokens in the app args as well. The docs state this behavior too.
And you can't replace the token inside the before_app_launch hook because only the cleaned version string is passed.

Sebastian Kral added 2 commits November 10, 2015 17:28
It makes sense to replace the {version} tokens in the app args as well
to make this even more flexible. You can't replace the token inside the
before_app_launch hook because only the cleaned version string is
getting passed.
@robblau
Copy link
Contributor

robblau commented Apr 5, 2016

Hi there, we're getting this functionality going and had a question about the fallback logic if versions are configured by the arg is None. What is the workflow that ends up in that situation?

Feels like if we do that for args we should do it everywhere to be consistent.

@robblau
Copy link
Contributor

robblau commented Apr 5, 2016

Ok... seeing consistent behavior with _get_app_path. More news soon on this PR soon.

return raw_app_args
else:
return self._translate_version_tokens(raw_app_args, version)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than duplicating the code that does the version replacement logic, how about breaking that out to it's own method as well? Something like this (untested):

    def _apply_version_to_setting(self, raw_string, version=None):
        """
        Replace any version tokens  contained in the raw_string with the appropriate version value from 
        the app settings. 

        If version is None, then no version has been specified and we will return the first version 
        from the versions list in the settings. If no versions have been defined in the settings, then 
        we return the raw_string since there's no replacement to do.

        :param raw_string: the raw string potentially containing the version tokens (eg. {version}, 
                           {v0}, ...) we will be replacing. This string could represent a number of 
                           things including a path, an args string, etc. 
        :param version: version string to use for the token replacement. 
        :returns: string with version tokens replaced with their appropriate values
        """
        if version is None:
            # there are two reasons version could be None
            # 1. if versions have not been configured, the raw string is assumed valid
            # 2. if versions has been configured, but no specific version was requested, then we 
            #    expand with the first element in the versions list, and use it as the default
            versions = self.get_setting("versions")
            if versions:
               return self._translate_version_tokens(raw_string, versions[0])
            else:
               return raw_string
        else:
            return self._translate_version_tokens(raw_string, version)

    def _get_app_path(self, version=None):
        """ Return the platform specific app path, performing version substitution. """
        platform_name = {"linux2": "linux", "darwin": "mac", "win32": "windows"}[sys.platform]
        raw_app_path = self.get_setting("%s_path" % platform_name, "")

        return self._apply_version_to_setting(raw_app_path)

    def _get_app_args(self, version=None):
        """ Return the platform specific app path, performing version substitution. """
        platform_name = {"linux2": "linux", "darwin": "mac", "win32": "windows"}[sys.platform]
        raw_app_args = self.get_setting("%s_args" % platform_name, "")

        return self._apply_version_to_setting(raw_app_args)

@kporangehat
Copy link

Sebastian,

I went ahead and made these changes in #30 and it's now in QA on our end. Thanks for the submission. Your approach was slightly different than my original idea and ultimately we liked yours better. Appreciate it! 🎉

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.

3 participants