Page MenuHomePhabricator

config: remove pycompat.bytestr() for defaultvalue
ClosedPublic

Authored by navaneeth.suresh on Aug 3 2019, 2:51 AM.

Details

Summary

This is a follow-up patch to 51a2e3102db2. This removes
pycompat.bytestr to preserve None in commands.config().

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

navaneeth.suresh edited the summary of this revision. (Show Details)Aug 3 2019, 2:55 AM

For some reasons, my previous comment seems to have never made it to phabricator:

You description says the changesets does three things. So it should be three different changesets. Can you split them ?

I don't understand why we are adding a warning here. The API usage seems valid.

durin42 resigned from this revision.Aug 7 2019, 8:09 AM
durin42 added a subscriber: durin42.

I agree with marmoute on all points.

navaneeth.suresh edited the summary of this revision. (Show Details)Aug 7 2019, 9:44 AM
navaneeth.suresh retitled this revision from config: fix defaultvalue template keyword to config: fix defaultvalue template keyword (patch 1 of 2).
navaneeth.suresh updated this revision to Diff 16148.
pulkit added a subscriber: pulkit.Aug 7 2019, 10:19 AM

The pycompat.bytestr() was there for py3 compatibility, however removing it should be fine in this case. Can you make sure test-config.t passes with this patch on Python 3?

This patch needs a better description.

navaneeth.suresh retitled this revision from config: fix defaultvalue template keyword (patch 1 of 2) to config: remove pycompat.bytestr() for defaultvalue.Aug 7 2019, 10:32 AM
In D6712#98424, @pulkit wrote:

The pycompat.bytestr() was there for py3 compatibility, however removing it should be fine in this case. Can you make sure test-config.t passes with this patch on Python 3?

Yes, test-config.t passes with this patch on py3.

pulkit accepted this revision.Aug 7 2019, 5:08 PM
This revision is now accepted and ready to land.Aug 7 2019, 5:08 PM
yuja added a subscriber: yuja.Aug 7 2019, 7:46 PM

The pycompat.bytestr() was there for py3 compatibility,

Not really for the "value" variable which may be unprintable object, but
I think it's okay to assume defaultvalues are None/bool/int/float/bytes type.