Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not modify frozen string #272

Merged
merged 10 commits into from Dec 3, 2014

Conversation

Projects
None yet
4 participants
@mchf
Copy link
Member

commented Nov 28, 2014

bnc#906694

:to => "list <integer>"
)
) do |key|
LanItems.Items.keys.each do |key|

This comment has been minimized.

Copy link
@jreidinger
let(:unknown_device_overview) {
["<ul><li><p>Unknown Network Device<br>Not configured yet.</p></li></ul>", []]
}
let(:deutsche_translation_overview) {

This comment has been minimized.

Copy link
@jreidinger

jreidinger Nov 28, 2014

Member

why using let? it looks like constant for me

This comment has been minimized.

Copy link
@mvidner

mvidner Nov 28, 2014

Member

Inconsistent language mix! Did you know sÿmböls cän hävë ümläüts ? :deutschübersetzungsübersicht

This comment has been minimized.

Copy link
@lslezak

lslezak Nov 28, 2014

Member

BTW avoid umlauts in variables/symbols, rubocop will complain later... 😉

@jreidinger

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

When I see this test and know how often this exception happen, then maybe it is good idea just to mock _ method to

def _(val)
  val.dup.freeze
end

this will simulate this problem.

@mchf mchf force-pushed the mchf:bnc906694-frozen-string branch from 66badaa to a81c8f4 Nov 28, 2014

@lslezak

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

Do we need the binary MO file (network.mo) in Git repo?

It is quite easy to convert the PO file, IIRC. The should be some (Auto)Make rule to create it easily...

@mchf mchf force-pushed the mchf:bnc906694-frozen-string branch from 1f50dca to 5681474 Nov 28, 2014

@@ -346,7 +346,7 @@ def DeviceStatus(devtype, devname, devmap)
host = NetHwDetection.ResolveIP(addr)
remip = Ops.get_string(devmap, "REMOTE_IPADDR", "")
if proto == "none"
return _("Configured without address (NONE)")
return _("Configured without address (NONE)").dup

This comment has been minimized.

Copy link
@mvidner

mvidner Nov 28, 2014

Member

I think this is the wrong way to fix it. APIs should be free to return frozen strings. Whoever needs a modifiable string should dup it themselves.

This comment has been minimized.

Copy link
@mchf

mchf Nov 28, 2014

Author Member

well, if the method can return aprox. ten different strings and only one can be frozen then it is at least unintuitive ...

@@ -1359,7 +1352,7 @@ def BuildLanOverview
links << href

This comment has been minimized.

Copy link
@mvidner

mvidner Nov 28, 2014

Member

... here:

status.dup << " " << warning << " " << Hyperlink(href, _("Change."))

This comment has been minimized.

Copy link
@mvidner

mvidner Nov 28, 2014

Member

Ugh. That is wrong because it discards the modified copy. so

status += " " << warning << " " << Hyperlink(href, _("Change."))
@mvidner

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

Is master the right target? IMHO it should be an update for 13.2 because the module is not usable for certain configs.

@mchf

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2014

in my POV, target is OS and SLE12-SP1

@mchf mchf force-pushed the mchf:bnc906694-frozen-string branch from 5681474 to 55e2fee Nov 28, 2014

@@ -586,7 +586,7 @@ def current_renamed_to

# Tells if current item was renamed
def renamed?(item_id)
return false if !@Items[item_id].has_key?("renamed_to")
return false if !LanItems.Items[item_id].has_key?("renamed_to")

This comment has been minimized.

Copy link
@mvidner

mvidner Nov 28, 2014

Member

I think repeating the class name (actually the object name) is clumsy and self.Items would work just as well.

This comment has been minimized.

Copy link
@jreidinger

jreidinger Nov 28, 2014

Member

actually also Items()[]works

@type,
NetworkInterfaces.device_num(NetworkInterfaces.Name),
NetworkInterfaces.Current
LanItems.type,

This comment has been minimized.

Copy link
@mvidner

mvidner Nov 28, 2014

Member

This could be just type, as long as we don't shadow the accessor with a local variable.

This comment has been minimized.

Copy link
@mchf

mchf Nov 28, 2014

Author Member

yes. That's I've "highlighted" that using LanItems. I don't believe anything when touching old code.

@mvidner

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

I have made some suggestions but I don't feel strongly about them. LGTM.

@lslezak

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

JFYI: I fixed the Travis build by fixing yast2 Ubuntu packaging at Yast:Head:Travis.

@mchf

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2014

Thanks for reviews.
Thanks @lslezak for help with Travis.

mchf added a commit that referenced this pull request Dec 3, 2014

@mchf mchf merged commit 47ae48d into yast:master Dec 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@mchf mchf deleted the mchf:bnc906694-frozen-string branch Dec 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.