<dobey> elopio: why did you make it so complicated?
<mandel> elopio, stupid comment, but 2011-2013 is good enough, right dobey?
dobey> mandel: eh?
<mandel> dobey, branch from elopio line 132 in the diff
<dobey> elopio: individual years is useful if some years were skipped. like "2008, 2012-2013" if nothing was changed in 2009 or 2010
<dobey> elopio: or if multiple contributors have different copyright in different years
<dobey> elopio: but since all copyright goes to canonical for most all of our projects, it's not necessary
<dobey> mandel: about gnu.org, or the branch review? :)
<elopio> dobey: it's just a small detail I got the custom to follow.
<mandel> dobey, gnu.org
<elopio> http://www.gnu.org/licenses/gpl-howto.html. It says that there must be an explicit satement about the usage of year ranges.
<elopio> but It shouldn't be a big deal.
<elopio> if you prefer, I can roll back to year ranges.
<dobey> elopio: well, i voted disapprove on the branch, because i think it's a whole lot of unnecessary change for what you want to accomplish
<elopio> dobey: I disagree, as the original approach treats as constants things that can vary depending on the environment.
<elopio> so, they are not constants.
<elopio> and it's missing the tests for the feature to override urls with env vars.
<dobey> python doesn't have constants
<dobey> missing tests is one thing. breaking API and turning everything into functions is unnecessary, even to test the things (though i don't think we should be writing tests to check that python's __dict__.get() method works correctly
<elopio> dobey: if you follow pep8, a variable all in capitals means a constant.
--> jfunk (<email address hidden>) has joined #u1-client
<elopio> dobey: I'm not testing pythons dict, I'm testing the environment variables.
<elopio> and I consider that my branch just changes the behavior introduced less than a month ago.
<dobey> it says "constants are usually" …
<elopio> dobey: and it's the only thing it recommends to use all in capitals.
<elopio> and for global variables it says, "Let's hope that these variables are meant for use inside one module only."
<dobey> regardless. there are no such things as constants in python. in normal usage the values of these constants will not change.
<elopio> dobey: I agree with the first part. Not with the second. A constant that sometimes changes, I think it's not a constant. And as variables, they should be lower case, and it's discouraged to import them in a different module. Thus, a function is the way to go, I think.
<dobey> that's irrelevant. it's still python, and in python there are no such things as constants
<dobey> and it's irrelevant to the complexity of change
<dobey> your change is overly complex for the desired change, and breaks existing API
<elopio> dobey: I can just roll back to rev 1029 of my branch to turn your disapproval, but I enjoy the discussion. I hope you don't see me as a pain in the ass for this, though I know the chances are high :D
<elopio> dobey: I agree it's more complex that just changing the strinsg.
<dobey> arguing about definition of constant in relation to python, and purity of following pep8 aren't useful discussions to have about this right now :)
<elopio> I don't see it as overly complex, because it adds test coverage, and makes use of the pep8 recommendations.
<elopio> at the price of breaking an api only published on a PPA for less than a month.
<dobey> i think you're confused about what api you're breaking
<elopio> and for free to you, as I did the complex change.
<elopio> dobey: um, yes, that sounds likely. Can you explain to me what API you are talking about?
<dobey> SERVICE_URL has been there for years
<elopio> ah, yes, you are right on that.
<elopio> sorry, I need coffee.
<dobey> and function calls are an unnecessary performance hit
<mandel> elopio, in theory, if it is public to use when importing from a python module, it is an API
<dobey> even if you wanted to be more purist about pep8/constants here, global variable would be the better way to go, rather than functions
<dobey> mandel: in actuality, if it's in python, and it's installed to /usr/lib/python*, it's an API :)
<elopio> ok, in my opinion, you introduced a bug by turning the constants into variables here: https://code.launchpad.net/~dobey/ubuntu-sso-client/testable-env/+merge/156080
<elopio> you would argue that python has no constants, and that it's not worth it breaking the API.
<dobey> elopio: you're saying i introduced a bug by implementing the feature you requested, which was to fix a bug that you could no longer use the previous "constant" that was being overwritten by an environment variable, due to the rebranding changes?
<elopio> dobey: yes, and without tests :) I'm not blaming you, I appreciated it a lot and it was really useful.
<dobey> i would argue that having environment variables and such to change core functionality, regardless of implementation details, is a pain in the ass, and a security flaw
<dobey> especially when the result is to replace https:// URLs with http:// ones
<elopio> I'm just pointing it out, and trying to fix what I consider broken because of the lack of tests and the wrong use of capitals. I don't agree with your reasons for disapproval, but I can live with this feature being untested.
<dobey> elopio: please stop conflating my disapproval due to complexity as a requirement that there be no tests
<elopio> dobey: the security should be covered by the servers, not by this client that can be easily hacked. So no security risk introduced. It's just easier to hack now.
<dobey> elopio: yeah, you go argue that one to the security team :)
<dobey> "the client doesn't need to be secure because the server should cover that"
<dobey> anyway
<dobey> you don't need to convert everything to functions to be able to test it
<dobey> and testing os.environ.get() is not a useful test
<elopio> I can go and argue that, with the security team, yes :)
<elopio> and I keep disagreeing with that. I'm not testing os.environ.get, that's actually patched.
<elopio> and I think that when something is takes a variable and does something with it to return a new value, like SERVICE_URL does now, it should be a function.
<elopio> however, it seems I'm not able to convince you of that.
<dobey> there's no need to write a function that wraps a single function call
<dobey> like i said, if you want to go that route, they should be global vars, not functions
<dobey> making it a function that calls a function induces a tiny performance hit (and they can add up when that function gets references many times)
<elopio> dobey: a global var would work for service_url, not for sso_auth_host as it's scope shouldn't go outside the module.
<mandel> elopio, in this case I agree with dobey, adding that extra wrap does not really bring anything to the table and it adds the performance cost
<mandel> elopio, also, it makes the code more error prone...
<mandel> elopio, this is going to sound a little crazy, but what about simply chaging the value of the vars that dobey added for the tests per testcase
<mandel> elopio, and assert things like you are doing right now (and no functions)
<mandel> elopio, use addCleanup to set the variable back to its original value once the test has been finished
<mandel> better to use addCleanup than tearDown because it will be executed always
<dobey> if you want to refactor things, i think we can discuss that as a seaparate thing, and not include it as part of the change to use a full base url instead of only host:port for the env var
<elopio> sorry, I'm back. mandel: global vars that don't go outside the module instead of functions sound just fine.
<elopio> dobey: and I think that's the only solution, as I need the feature and I'm not able to convince.
<elopio> I'll roll-back to the simple solution, and propose a different branch.
<dobey> elopio: individual years is useful if some years were skipped. like "2008, 2012-2013" if nothing was changed in 2009 or 2010
<dobey> elopio: or if multiple contributors have different copyright in different years
<dobey> elopio: but since all copyright goes to canonical for most all of our projects, it's not necessary
<dobey> mandel: about gnu.org, or the branch review? :)
<elopio> dobey: it's just a small detail I got the custom to follow.
<mandel> dobey, gnu.org
<elopio> http://www.gnu.org/licenses/gpl-howto.html. It says that there must be an explicit satement about the usage of year ranges.
<elopio> but It shouldn't be a big deal.
<elopio> if you prefer, I can roll back to year ranges.
<dobey> elopio: well, i voted disapprove on the branch, because i think it's a whole lot of unnecessary change for what you want to accomplish
<elopio> dobey: I disagree, as the original approach treats as constants things that can vary depending on the environment.
<elopio> so, they are not constants.
<elopio> and it's missing the tests for the feature to override urls with env vars.
<dobey> python doesn't have constants
<dobey> missing tests is one thing. breaking API and turning everything into functions is unnecessary, even to test the things (though i don't think we should be writing tests to check that python's __dict__.get() method works 
correctly
<elopio> dobey: if you follow pep8, a variable all in capitals means a constant.
--> jfunk (<email address hidden>) has joined #u1-client
<elopio> dobey: I'm not testing pythons dict, I'm testing the environment variables.
<elopio> and I consider that my branch just changes the behavior introduced less than a month ago.
<dobey> it says "constants are usually" …
<elopio> dobey: and it's the only thing it recommends to use all in capitals.
<elopio> and for global variables it says, "Let's hope that these variables are meant for use inside one module only."
<dobey> regardless. there are no such things as constants in python. in normal usage the values of these constants will not change.
<elopio> dobey: I agree with the first part. Not with the second. A constant that sometimes changes, I think it's not a constant. And as variables, they should be lower case, and it's discouraged to import them in a different module. Thus, a function is the way to go, I think.
<dobey> that's irrelevant. it's still python, and in python there are no such things as constants
<dobey> and it's irrelevant to the complexity of change
<dobey> your change is overly complex for the desired change, and breaks existing API
<elopio> dobey: I can just roll back to rev 1029 of my branch to turn your disapproval, but I enjoy the discussion. I hope you don't see me as a pain in the ass for this, though I know the chances are high :D
<elopio> dobey: I agree it's more complex that just changing the strinsg.
<dobey> arguing about definition of constant in relation to python, and purity of following pep8 aren't useful discussions to have about this right now :)
<elopio> I don't see it as overly complex, because it adds test coverage, and makes use of the pep8 recommendations.
<elopio> at the price of breaking an api only published on a PPA for less than a month.
<dobey> i think you're confused about what api you're breaking
<elopio> and for free to you, as I did the complex change.
<elopio> dobey: um, yes, that sounds likely. Can you explain to me what API you are talking about?
<dobey> SERVICE_URL has been there for years
<elopio> ah, yes, you are right on that.
<elopio> sorry, I need coffee.
<dobey> and function calls are an unnecessary performance hit
<mandel> elopio, in theory, if it is public to use when importing from a python module, it is an API
<dobey> even if you wanted to be more purist about pep8/constants here, global variable would be the better way to go, rather than functions
<dobey> mandel: in actuality, if it's in python, and it's installed to /usr/lib/python*, it's an API :)
<elopio> ok, in my opinion, you introduced a bug by turning the constants into variables here: https://code.launchpad.net/~dobey/ubuntu-sso-client/testable-env/+merge/156080
<elopio> you would argue that python has no constants, and that it's not worth it breaking the API.
<dobey> elopio: you're saying i introduced a bug by implementing the feature you requested, which was to fix a bug that you could no longer use the previous "constant" that was being overwritten by an environment variable, due to the rebranding changes?
<elopio> dobey: yes, and without tests :) I'm not blaming you, I appreciated it a lot and it was really useful.
<dobey> i would argue that having environment variables and such to change core functionality, regardless of implementation details, is a pain in the ass, and a security flaw
<dobey> especially when the result is to replace https:// URLs with http:// ones
<elopio> I'm just pointing it out, and trying to fix what I consider broken because of the lack of tests and the wrong use of capitals. I don't agree with your reasons for disapproval, but I can live with this feature being untested.
<dobey> elopio: please stop conflating my disapproval due to complexity as a requirement that there be no tests
<elopio> dobey: the security should be covered by the servers, not by this client that can be easily hacked. So no security risk introduced. It's just easier to hack now.
<dobey> elopio: yeah, you go argue that one to the security team :)
<dobey> "the client doesn't need to be secure because the server should cover that"
<dobey> anyway
<dobey> you don't need to convert everything to functions to be able to test it
<dobey> and testing os.environ.get() is not a useful test
<elopio> I can go and argue that, with the security team, yes :)
<elopio> and I keep disagreeing with that. I'm not testing os.environ.get, that's actually patched.
<elopio> and I think that when something is takes a variable and does something with it to return a new value, like SERVICE_URL does now, it should be a function.
<elopio> however, it seems I'm not able to convince you of that.
<dobey> there's no need to write a function that wraps a single function call
<dobey> like i said, if you want to go that route, they should be global vars, not functions
<dobey> making it a function that calls a function induces a tiny performance hit (and they can add up when that function gets references many times)
<elopio> dobey: a global var would work for service_url, not for sso_auth_host as it's scope shouldn't go outside the module.
<mandel> elopio, in this case I agree with dobey, adding that extra wrap does not really bring anything to the table and it adds the performance cost
<mandel> elopio, also, it makes the code more error prone...
<mandel> elopio, this is going to sound a little crazy, but what about simply chaging the value of the vars that dobey added for the tests per testcase
<mandel> elopio, and assert things like you are doing right now (and no functions)
<mandel> elopio, use addCleanup to set the variable back to its original value once the test has been finished
<mandel> better to use addCleanup than tearDown because it will be executed always
<dobey> if you want to refactor things, i think we can discuss that as a seaparate thing, and not include it as part of the change to use a full base url instead of only host:port for the env var
<elopio> sorry, I'm back. mandel: global vars that don't go outside the module instead of functions sound just fine.
<elopio> dobey: and I think that's the only solution, as I need the feature and I'm not able to convince.
<elopio> I'll roll-back to the simple solution, and propose a different branch.