[Openlp-core] [Bug 1751626] Re: -d flag is used in a very confusing way
** Changed in: openlp Milestone: None => 2.9.1 ** Changed in: openlp Status: In Progress => Fix Committed -- You received this bug notification because you are a member of OpenLP Core, which is subscribed to OpenLP. https://bugs.launchpad.net/bugs/1751626 Title: -d flag is used in a very confusing way Status in OpenLP: Fix Committed Bug description: in the startup code the parameters accepted include --dev-version (-d). What's irritating is that it's not actually used there. The values are also not actually passed along somewhere else. However the about dialog still correctly displays the bzr revision if started with -d. So a little searching showed the following. The argument is defined in openlp.core.app.parse_options but only used in openlp.core.version.get_version Since the dev_version is not stored somewhere or passed to some place, get_version actually checks sys.argv searching for "-d" or "--dev- version" This is not very nice to people reading the code. It very much looks like the argument definition is not actually used. My request: * either completely remove the functionality * or properly use the parsed value, so it's clear it actually does something. also having the bzr check code in openlp.core.version.get_version is probably not the best idea. Especially since the code includes a comment that it's a duplicate of code in setup.py Further rants: in setup.py the code is not actually exactly the same, there are modifications and there is a comment that it is a duplicate of the no longer existing openlp.core.common.checkversions.py This illustrates that comments are bad for the reason that they are often not updated when the code changes. This can lead to comments confusing more than helping. In short: Remove the flag, remove the code from get_version and only have it in setup.py The flag is only useful for people running from bzr so devs, which should know what they are running and even if they don't they should know how to get that information without reverting to the about tab. To manage notifications about this bug go to: https://bugs.launchpad.net/openlp/+bug/1751626/+subscriptions ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
[Openlp-core] [Bug 1751626] Re: -d flag is used in a very confusing way
** Changed in: openlp Assignee: (unassigned) => Tim Bentley (trb143) ** Changed in: openlp Status: Won't Fix => In Progress ** Changed in: openlp Importance: Undecided => Low -- You received this bug notification because you are a member of OpenLP Core, which is subscribed to OpenLP. https://bugs.launchpad.net/bugs/1751626 Title: -d flag is used in a very confusing way Status in OpenLP: In Progress Bug description: in the startup code the parameters accepted include --dev-version (-d). What's irritating is that it's not actually used there. The values are also not actually passed along somewhere else. However the about dialog still correctly displays the bzr revision if started with -d. So a little searching showed the following. The argument is defined in openlp.core.app.parse_options but only used in openlp.core.version.get_version Since the dev_version is not stored somewhere or passed to some place, get_version actually checks sys.argv searching for "-d" or "--dev- version" This is not very nice to people reading the code. It very much looks like the argument definition is not actually used. My request: * either completely remove the functionality * or properly use the parsed value, so it's clear it actually does something. also having the bzr check code in openlp.core.version.get_version is probably not the best idea. Especially since the code includes a comment that it's a duplicate of code in setup.py Further rants: in setup.py the code is not actually exactly the same, there are modifications and there is a comment that it is a duplicate of the no longer existing openlp.core.common.checkversions.py This illustrates that comments are bad for the reason that they are often not updated when the code changes. This can lead to comments confusing more than helping. In short: Remove the flag, remove the code from get_version and only have it in setup.py The flag is only useful for people running from bzr so devs, which should know what they are running and even if they don't they should know how to get that information without reverting to the about tab. To manage notifications about this bug go to: https://bugs.launchpad.net/openlp/+bug/1751626/+subscriptions ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
[Openlp-core] [Bug 1751626] Re: -d flag is used in a very confusing way
If you are ok with it, I would create a merge request that removes it. -- You received this bug notification because you are a member of OpenLP Core, which is subscribed to OpenLP. https://bugs.launchpad.net/bugs/1751626 Title: -d flag is used in a very confusing way Status in OpenLP: Won't Fix Bug description: in the startup code the parameters accepted include --dev-version (-d). What's irritating is that it's not actually used there. The values are also not actually passed along somewhere else. However the about dialog still correctly displays the bzr revision if started with -d. So a little searching showed the following. The argument is defined in openlp.core.app.parse_options but only used in openlp.core.version.get_version Since the dev_version is not stored somewhere or passed to some place, get_version actually checks sys.argv searching for "-d" or "--dev- version" This is not very nice to people reading the code. It very much looks like the argument definition is not actually used. My request: * either completely remove the functionality * or properly use the parsed value, so it's clear it actually does something. also having the bzr check code in openlp.core.version.get_version is probably not the best idea. Especially since the code includes a comment that it's a duplicate of code in setup.py Further rants: in setup.py the code is not actually exactly the same, there are modifications and there is a comment that it is a duplicate of the no longer existing openlp.core.common.checkversions.py This illustrates that comments are bad for the reason that they are often not updated when the code changes. This can lead to comments confusing more than helping. In short: Remove the flag, remove the code from get_version and only have it in setup.py The flag is only useful for people running from bzr so devs, which should know what they are running and even if they don't they should know how to get that information without reverting to the about tab. To manage notifications about this bug go to: https://bugs.launchpad.net/openlp/+bug/1751626/+subscriptions ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
[Openlp-core] [Bug 1751626] Re: -d flag is used in a very confusing way
I'm leaning toward agreeing with Simon. The upgrade tests should just mock out the version number (that's why we use mock in the first place), and I'm pretty sure they do already. At minimum I'd suggest using the flags in the registry instead of depending on sys.argv, and collecting the bzr version code to a single place that is used in both setup.py and OpenLP as a whole. -- You received this bug notification because you are a member of OpenLP Core, which is subscribed to OpenLP. https://bugs.launchpad.net/bugs/1751626 Title: -d flag is used in a very confusing way Status in OpenLP: Won't Fix Bug description: in the startup code the parameters accepted include --dev-version (-d). What's irritating is that it's not actually used there. The values are also not actually passed along somewhere else. However the about dialog still correctly displays the bzr revision if started with -d. So a little searching showed the following. The argument is defined in openlp.core.app.parse_options but only used in openlp.core.version.get_version Since the dev_version is not stored somewhere or passed to some place, get_version actually checks sys.argv searching for "-d" or "--dev- version" This is not very nice to people reading the code. It very much looks like the argument definition is not actually used. My request: * either completely remove the functionality * or properly use the parsed value, so it's clear it actually does something. also having the bzr check code in openlp.core.version.get_version is probably not the best idea. Especially since the code includes a comment that it's a duplicate of code in setup.py Further rants: in setup.py the code is not actually exactly the same, there are modifications and there is a comment that it is a duplicate of the no longer existing openlp.core.common.checkversions.py This illustrates that comments are bad for the reason that they are often not updated when the code changes. This can lead to comments confusing more than helping. In short: Remove the flag, remove the code from get_version and only have it in setup.py The flag is only useful for people running from bzr so devs, which should know what they are running and even if they don't they should know how to get that information without reverting to the about tab. To manage notifications about this bug go to: https://bugs.launchpad.net/openlp/+bug/1751626/+subscriptions ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
[Openlp-core] [Bug 1751626] Re: -d flag is used in a very confusing way
I know that it works as intended. Where are the tests for automated upgrades and how do they use this functionality? This is mainly a code style and code quality issue. And is a maintenance nightmare. If you want to keep it, make the code exist only once and actually use the parsed argument. The comments you made don't address my real issue with this. So I think closing it like this is not the correct answer. -- You received this bug notification because you are a member of OpenLP Core, which is subscribed to OpenLP. https://bugs.launchpad.net/bugs/1751626 Title: -d flag is used in a very confusing way Status in OpenLP: Won't Fix Bug description: in the startup code the parameters accepted include --dev-version (-d). What's irritating is that it's not actually used there. The values are also not actually passed along somewhere else. However the about dialog still correctly displays the bzr revision if started with -d. So a little searching showed the following. The argument is defined in openlp.core.app.parse_options but only used in openlp.core.version.get_version Since the dev_version is not stored somewhere or passed to some place, get_version actually checks sys.argv searching for "-d" or "--dev- version" This is not very nice to people reading the code. It very much looks like the argument definition is not actually used. My request: * either completely remove the functionality * or properly use the parsed value, so it's clear it actually does something. also having the bzr check code in openlp.core.version.get_version is probably not the best idea. Especially since the code includes a comment that it's a duplicate of code in setup.py Further rants: in setup.py the code is not actually exactly the same, there are modifications and there is a comment that it is a duplicate of the no longer existing openlp.core.common.checkversions.py This illustrates that comments are bad for the reason that they are often not updated when the code changes. This can lead to comments confusing more than helping. In short: Remove the flag, remove the code from get_version and only have it in setup.py The flag is only useful for people running from bzr so devs, which should know what they are running and even if they don't they should know how to get that information without reverting to the about tab. To manage notifications about this bug go to: https://bugs.launchpad.net/openlp/+bug/1751626/+subscriptions ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
[Openlp-core] [Bug 1751626] Re: -d flag is used in a very confusing way
This works fine and does provide so real functionality. without the -d file shows the version from the .version file which is controlled from bzr code and matches the main code version. with the -d uses the version from bzr and changes the version information using bzr tags. This is useful for when testing automated upgrades. Unless you are a dev or running code versions you will not know about this so it is not an issue. ** Changed in: openlp Status: New => Won't Fix -- You received this bug notification because you are a member of OpenLP Core, which is subscribed to OpenLP. https://bugs.launchpad.net/bugs/1751626 Title: -d flag is used in a very confusing way Status in OpenLP: Won't Fix Bug description: in the startup code the parameters accepted include --dev-version (-d). What's irritating is that it's not actually used there. The values are also not actually passed along somewhere else. However the about dialog still correctly displays the bzr revision if started with -d. So a little searching showed the following. The argument is defined in openlp.core.app.parse_options but only used in openlp.core.version.get_version Since the dev_version is not stored somewhere or passed to some place, get_version actually checks sys.argv searching for "-d" or "--dev- version" This is not very nice to people reading the code. It very much looks like the argument definition is not actually used. My request: * either completely remove the functionality * or properly use the parsed value, so it's clear it actually does something. also having the bzr check code in openlp.core.version.get_version is probably not the best idea. Especially since the code includes a comment that it's a duplicate of code in setup.py Further rants: in setup.py the code is not actually exactly the same, there are modifications and there is a comment that it is a duplicate of the no longer existing openlp.core.common.checkversions.py This illustrates that comments are bad for the reason that they are often not updated when the code changes. This can lead to comments confusing more than helping. In short: Remove the flag, remove the code from get_version and only have it in setup.py The flag is only useful for people running from bzr so devs, which should know what they are running and even if they don't they should know how to get that information without reverting to the about tab. To manage notifications about this bug go to: https://bugs.launchpad.net/openlp/+bug/1751626/+subscriptions ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
