Opened at 2012-11-08T01:49:22Z

Closed at 2014-04-21T22:05:56Z

Last modified at 2014-04-22T14:47:23Z

#1847 closed defect (fixed)

Ugly shadowing of Client.DEFAULT_ENCODING_PARAMETERS

Reported by: davidsarah Owned by: daira
Priority: minor Milestone: 1.10.1
Component: code-encoding Version: 1.9.2
Keywords: reviewed Cc:
Launchpad Bug:

Description (last modified by daira)

Client in git/src/allmydata/client.py has a class attribute called DEFAULT_ENCODING_PARAMETERS, and an instance attribute also called DEFAULT_ENCODING_PARAMETERS that shadows it. This is ugly and confusing.

The linked pull request changes the name of the latter to _encoding_parameters.

Change History (30)

comment:1 Changed at 2012-11-08T02:11:38Z by davidsarah

  • Keywords review-needed added
  • Owner changed from davidsarah to warner

​https://github.com/tahoe-lafs/tahoe-lafs/pull/18

comment:2 Changed at 2012-11-08T16:19:01Z by davidsarah

  • Keywords review-needed removed
  • Owner changed from warner to davidsarah
  • Status changed from new to assigned

Hmm, given the amount of test code that sets encoding parameters, it may be worth adding a set_encoding_parameters method to client. So please hold off merging this.

Version 0, edited at 2012-11-08T16:19:01Z by davidsarah (next)

comment:3 follow-up: Changed at 2013-04-10T06:36:22Z by zooko

Could this be related to #1830?

comment:4 in reply to: ↑ 3 Changed at 2013-06-20T11:40:04Z by daira

  • Description modified (diff)

Replying to zooko:

Could this be related to #1830?

No, see 1830#comment:5.

comment:5 follow-up: Changed at 2013-08-13T23:37:42Z by zooko

#1830 is still bothering me (see comment:6:ticket:1830), and fixing this ticket would help because then I wouldn't be distracted by the nagging feeling that this ugly shadowing is somehow interacting with #1830. That is: fixing this ugly shadowing would help me think about #1830, even if the ugly shadowing is not actually causing #1830. ☺ (Or even if, as comment:6:ticket:1830 suggests, #1830 is actually already fixed!)

comment:6 Changed at 2013-08-31T17:08:38Z by daira

  • Milestone changed from soon to 1.11.0
  • Owner changed from davidsarah to daira
  • Status changed from assigned to new

comment:7 in reply to: ↑ 5 Changed at 2013-08-31T17:09:46Z by daira

  • Status changed from new to assigned

Replying to zooko:

#1830 is still bothering me (see comment:6:ticket:1830), and fixing this ticket would help because then I wouldn't be distracted by the nagging feeling that this ugly shadowing is somehow interacting with #1830.

+1.

comment:8 Changed at 2013-11-01T17:03:32Z by zooko

comment:12:ticket:549 is an example of how this default/shadowing/fall-back code gets in the way of work.

comment:9 Changed at 2013-11-02T06:49:52Z by bazuka

  • Owner changed from daira to bazuka
  • Status changed from assigned to new

comment:10 Changed at 2013-11-02T06:50:12Z by bazuka

  • Status changed from new to assigned

comment:11 Changed at 2013-11-02T09:00:54Z by bazuka

Hi I have made the changes, please review the changes. review-needed: ​https://github.com/tahoe-lafs/tahoe-lafs/pull/65

All test cases are green.

comment:12 Changed at 2013-11-03T18:48:34Z by daira

[copying github comment here]

There's a cscope.out file that is added and then deleted. Please squash the last two commits so that this file doesn't appear and the commits are easier to review.

comment:13 Changed at 2013-11-03T18:49:30Z by daira

Also, the fix for this ticket should be independent of #549.

comment:15 Changed at 2013-11-26T19:09:18Z by bazuka

  • Component changed from code-encoding to code
  • Keywords review-needed added; cleanup removed

comment:16 Changed at 2013-11-26T19:23:26Z by daira

  • Component changed from code to code-encoding
  • Keywords review-needed removed

This still has the cscope.out file. Also, when the indentation of the first line of a dict literal changes, please change the remaining lines to match.

comment:18 Changed at 2013-11-26T21:14:09Z by bazuka

  • Keywords review-needed added

comment:19 Changed at 2013-11-27T01:38:26Z by daira

  • Owner changed from bazuka to daira
  • Status changed from assigned to new

Reviewing.

comment:20 follow-up: Changed at 2013-11-27T01:38:33Z by daira

  • Status changed from new to assigned

comment:21 in reply to: ↑ 20 Changed at 2013-12-03T15:48:54Z by bazuka

Replying to daira: daira is there any changes I need to do?

comment:22 Changed at 2013-12-03T20:33:31Z by nsh

reviewed code in last commit. seems fine to my (very fresh[!]) eyes...

comment:23 follow-up: Changed at 2013-12-03T21:36:46Z by zooko

  • Status changed from assigned to new

I just reviewed this, too, comparing ​https://github.com/basu123/tahoe-lafs/commit/51dcecd01a730f588ce439a013d19c67f8f131b9#commitcomment-4763253 to ​https://github.com/daira/tahoe-lafs/commit/ab9216e5728083a671060ba12c98f56638f08517 . I had one question about a way that these two patches differ:

​https://github.com/basu123/tahoe-lafs/commit/51dcecd01a730f588ce439a013d19c67f8f131b9#commitcomment-4763253

Also, wiki:CodingStandards says "Prepend a leading underscore to private names.", so the way ​https://github.com/daira/tahoe-lafs/commit/ab9216e5728083a671060ba12c98f56638f08517 did that is preferred.

Otherwise, I don't see any problem with either of these two patches and either one can be merged to trunk in my opinion.

comment:24 Changed at 2013-12-03T21:37:01Z by zooko

  • Keywords review-needed removed

removing the review-needed flag

comment:25 Changed at 2013-12-03T21:37:35Z by zooko

  • Keywords reviewed added

adding the "reviewed" flag to let it be known that either of these two branches could be merged to trunk.

comment:26 in reply to: ↑ 23 Changed at 2013-12-04T01:32:03Z by daira

Replying to zooko:

Also, wiki:CodingStandards says "Prepend a leading underscore to private names.", so the way ​https://github.com/daira/tahoe-lafs/commit/ab9216e5728083a671060ba12c98f56638f08517 did that is preferred.

I disagree, because these attributes aren't private. (I count attributes as non-private if they are accessed directly from anywhere outside the class, including tests.) The patch looks fine to me in this respect; I had originally planned to add a method to set parameters, but I think it's not needed.

Last edited at 2014-04-14T22:11:28Z by daira (previous) (diff)

comment:27 Changed at 2014-04-21T21:52:48Z by Daira Hopwood <daira@…>

In 4889129f3732219cb9cedb1eb27dec3da3f22db2/trunk:

Minor comment fix. refs #1847

Signed-off-by: Daira Hopwood <daira@…>

comment:28 Changed at 2014-04-21T22:05:56Z by daira

  • Resolution set to fixed
  • Status changed from new to closed

Fixed by [77767e9e12cbd3e03f8ad917f6d3e8c1e4918c43/trunk]. (See #2231 for why that didn't show up here automatically.)

comment:29 Changed at 2014-04-21T22:42:18Z by Daira Hopwood <daira@…>

In 4889129f3732219cb9cedb1eb27dec3da3f22db2/trunk:

Minor comment fix. refs #1847

Signed-off-by: Daira Hopwood <daira@…>

comment:30 Changed at 2014-04-22T14:47:23Z by Daira Hopwood <daira@…>

In 4889129f3732219cb9cedb1eb27dec3da3f22db2/trunk:

Minor comment fix. refs #1847

Signed-off-by: Daira Hopwood <daira@…>

Note: See TracTickets for help on using tickets.