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
Lint Skipped
Unit
Unit Tests Skipped

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.