Merge lp://staging/~elopio/ubuntu-sso-client/scheme_in_env into lp://staging/ubuntu-sso-client

Proposed by Leo Arias
Status: Merged
Approved by: dobey
Approved revision: 1036
Merged at revision: 1029
Proposed branch: lp://staging/~elopio/ubuntu-sso-client/scheme_in_env
Merge into: lp://staging/ubuntu-sso-client
Diff against target: 87 lines (+12/-10)
4 files modified
ubuntu_sso/__init__.py (+4/-2)
ubuntu_sso/account.py (+2/-2)
ubuntu_sso/qt/sso_wizard_page.py (+4/-4)
ubuntu_sso/utils/webclient/timestamp.py (+2/-2)
To merge this branch: bzr merge lp://staging/~elopio/ubuntu-sso-client/scheme_in_env
Reviewer Review Type Date Requested Status
dobey (community) Approve
Review via email: mp+159513@code.staging.launchpad.net

Commit message

Changed the environment variables for sso auth and uone from hosts to urls.

To post a comment you must log in.
1034. By Leo Arias

The variable constants are no longer needed.

1035. By Leo Arias

Removed unused method.

Revision history for this message
dobey (dobey) wrote :

This is an overly complex change in order to accomplish the desired outcome. I see no need to do all this extra work by moving things to functions rather than constants (which may be imported/used by other applications and libraries as well). There's no need to break API to support the environment variable including the URL scheme as well as the host and port.

review: Disapprove
Revision history for this message
Leo Arias (elopio) wrote :
Download full text (15.2 KiB)

<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 usef...

1036. By Leo Arias

Revert to rev 1029.

Revision history for this message
dobey (dobey) wrote :

With all the refactoring gone, this looks OK now.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches