rubocop update #83

jreidinger · 2016-07-15T08:45:53Z

No description provided.

coveralls · 2016-07-15T08:50:05Z

Coverage decreased (-18.6%) to 54.216% when pulling 7348219 on rubocop-update into f4f1fad on master.

aschnell · 2016-07-15T08:52:49Z

src/lib/storage/fake_device_factory.rb

@@ -212,9 +212,9 @@ def set_topology_attributes!(disk, args)
        io_size = args["io_size"]
        min_grain = args["min_grain"]
        align_ofs = args["align_ofs"]
-        disk.topology.optimal_io_size = io_size.size if io_size && io_size.size > 0
+        disk.topology.optimal_io_size = io_size.size if io_size && !io_size.empty?


Is that really readable? What does empty mean for a size? A house can be empty, but a size?

"empty for a size" ? And what does "size for size" mean as in the original?

Anyway, if something has size greater than zero than it's not empty...
For me it's better readable.

I agree, that size looks strange here, but also as @lslezak mention, io_size.size It looks like io_size is not good name...maybe it is number of available blocks? what it actually strands for?

io_size is the optimal input/output size of the disk. So it really represents a size. Technically it is a DiskSize object. The DiskSize object has a zero? method but not an empty? method (which is correct).

AFAIS the change breaks the testsuite:

undefined method `empty?' for <DiskSize 2.00 GiB (2147483648)>:Yast::Storage::DiskSize

lslezak · 2016-07-15T09:07:20Z

LGTM

aschnell · 2016-07-15T09:11:48Z

LGTM with all checks failing and coverage decreasing by 18%?

lslezak · 2016-07-15T09:22:24Z

Ouch sorry, overlooked that failures, canceling my LGTM... 😉

jreidinger · 2016-07-15T11:00:37Z

yep, I know about failing tests. Decrease of coverage is caused by failing tests, but from my initial look, I do not see any obvious reason why it failing, so I plan to wait for storage-ng experts for hints, what that exception and missing methods means as from diff I do not see something wrong.

aschnell · 2016-07-15T12:18:54Z

Explanation given above, DiskSize has no empty? method. Was that change done automatically by rubocop?

lslezak · 2016-07-15T12:28:40Z

I guess it was changed automatically by Rubocop.

So we have two options:

Add empty? method
Disable that check

I do not know which one is better in this case...

jreidinger · 2016-07-15T13:36:49Z

yes, automatic one as rubocop expects that when object have size method, it also have empty?.
Still even when explained, I found quite strange to have size method for Size object. Maybe method name like to_i ( which indicate conversion to plan integer value ) is better name?

aschnell · 2016-07-15T14:21:55Z

Assuming size == 0 is equal to empty? is terrible wrong. E.g. you could define size for a volume group to be the size in bytes and empty? to mean that it has no logical volumes. Those definitions are totally valid.

I vote to disable that check.

lslezak · 2016-07-15T15:09:26Z

Um yes, you could define empty? with a completely different meaning, unrelated to size. But I'd avoid that, that would be quite confusing. In that case you could define a separate method e.g. volumes and call empty on it (volumes.empty?). BTW even C++ STL defines empty() related to the size()...

So to get forward - if empty? could have a different meaning here or should not be defined at all then let's disable the check.

jreidinger · 2016-07-18T07:24:27Z

I disabled zero predicate checker and revert change. But it still failing with strange StorageException. Not very helpful to find what exactly failing and why. @ancorgs any idea how to get more info from failed tests?

coveralls · 2016-07-18T07:25:51Z

Coverage decreased (-18.6%) to 54.216% when pulling b268f06 on rubocop-update into f4f1fad on master.

aschnell · 2016-07-18T07:35:47Z

The size -> empty? is still done many times:

undefined method `empty?' for <DiskSize 2.00 GiB (2147483648)>:Yast::Storage::DiskSize

jreidinger · 2016-07-18T07:45:08Z

@aschnell ok, I will check all changes for the size

coveralls · 2016-07-18T08:13:44Z

Coverage increased (+0.07%) to 72.909% when pulling 2ec32dc on rubocop-update into f4f1fad on master.

lslezak · 2016-07-19T08:01:20Z

@jreidinger thanks for updating it.

The last unresolved thing is that casecmp("ms-dos").zero? part...

How should we proceed? I already commented this change above and I do not have a strong opinion here.

But I'd probably slightly more prefer the new style with casecmp to less diverge from the upstream style
although it's zero? part is a bit less readable. On the other hand the casecmp part is quite self descriptive and you can guess what it does...

@jreidinger, @aschnell, others... ❓❓

aschnell · 2016-07-19T08:53:40Z

Since I'm not the original author of the code I let @shundhammer and @ancorgs have the last word on the casecmp.zero? issue.

jreidinger · 2016-07-19T08:56:15Z

same for me as long as it is consistent

wfeldt · 2016-07-19T11:25:28Z

BTW, there's a DiskSize#to_i (aliased to #size) if #size irks you.

Whatever the rationale behind appending .freeze may be, I will not be tempted to comment on that.

jreidinger · 2016-07-19T11:31:35Z

@wfeldt rationale behind .freeze, is that it make constant really constant, so if something modify it, it raise exception and not silently modify it.

jreidinger · 2016-07-19T11:32:21Z

@wfeldt yast, DiskSize#to_i make much more sense to me, as size method for me indicate that it returns Size object.

wfeldt · 2016-07-19T11:49:11Z

@jreidinger, I said I will not comment on .freeze (because one should not use bad language publicly), not that I can't imagine why it's used.

wfeldt · 2016-07-19T11:49:57Z

lngtm, I stongly object to the .freeze.

ancorgs · 2016-07-19T11:52:32Z

I don't like the freeze either.

ancorgs · 2016-07-19T14:04:18Z

src/lib/storage/abstract_device_factory.rb

@@ -300,7 +296,6 @@ def check_param(name, param)
      # @param child  [String] name of child  factory product
      #
      def check_hierarchy(parent, child)
-        # rubocop:disable Style/GuardClause


For consistency, remove also this four lines below

# rubocop:enable Style/GuardClause

it is actually auto correction from rubocop, probably only one half is implemented

ancorgs · 2016-07-19T14:43:08Z

src/lib/storage/proposal/space_maker.rb

          end
-          log.info "Didn't manage to free enough space by resizing Windows"
+
+          log.info "Didn't manage to free enough space by resizing Windows" unless success


In this method, I think the original version makes the intention of the code more obvious when compared to the modified version. It would be nice if somebody else than @shundhammer or me do the exercise of reading both versions to confirm my feelings.

well, the most readable would be to move whole shrinking to own method, to my taste this method is too long, but I do not like to do big refactoring. BTW I agree with rubocop, that having return inside block is quite tricky workflow, that prevents future refactoring, as moving this block to own method then need special handling for such "long jump".

something like

success = sorted_resizables(parts_by_disk.values.flatten).any?(&:try_shrink) log.info "Didn't manage to free enough space by resizing Windows" unless success

btw also sorted_resizables(parts_by_disk.values.flatten) is not nice api, I expect something like disks.partitions_with_windows.reduce(:+).sort_by(&:size).any? which is more readable for me what it actually probably do ( give me disks, their windows partitions and sort it by size )

Well, it's true that removing the return from the inner block makes refactoring much easier. You convinced me, let's do the change (we can do the refactoring in a separate PR).

coveralls · 2016-07-19T14:46:10Z

Coverage increased (+0.07%) to 72.909% when pulling 875f437 on rubocop-update into f4f1fad on master.

lslezak · 2016-07-20T11:17:33Z

The new syntax won in the case insensitive poll 5:0, so let's use the new syntax.

I'm giving my LGTM.

The discussion is quite long but it seems everything is resolved now and we can merge it. If I overlooked something then react ASAP please.

lslezak · 2016-07-20T11:36:40Z

Maybe one more question: Do we agree on the freeze usage? We described the reasons in this and the following comments, is it OK for you? Do you agree with using freeze or do you still think we need to discuss it more?

wfeldt · 2016-07-20T11:58:08Z

Sorry, there was nothing putting me in favor of .freeze.

If you see code where it helps you, fine with me. But I'm against adding it to every constant unconditionally.

lslezak · 2016-07-20T13:13:29Z

@wfeldt um, it's hard to say where it helps until you actually use it.

It's like a foo(const string &bar) method, it would work exactly the same way also after removing that const.

But it is still a good practice to have it there as it prevents from programming mistakes when somebody modifies bar by mistake. It's also used unconditionally everywhere.

It's the same situation here in Ruby. It's just a bit more tricky because constants are actually not real constants as someone would expect.

ancorgs · 2016-07-21T09:30:33Z

It may not be the most well-argued opinion but... I simply hate that freeze change. 😉

wfeldt · 2016-07-21T09:42:58Z

@ancorgs: I really like your argument ;-)

ancorgs · 2016-07-22T07:45:45Z

We need an agreement here. I want to merge new code this morning. Should we vote?

lslezak · 2016-07-22T08:10:58Z

OK, so let's vote, use the reaction button to vote here this way:

👍 for the new style (with freeze)
👎 for the old style (without freeze)

lslezak · 2016-07-22T08:19:01Z

I'm not much happy about adding the freeze attribute, but given that it's for the future compatibility and one day (Ruby 3.0?) we can remove it possibly with a simple rubocop -a I voted for the new style.

JFYI in the other YaST modules we already switched to the new style, if the old style wins here we might need to update several packages to be consistent...

ancorgs · 2016-07-22T08:27:51Z

To be honest, Ruby constants work as I would have expected (defining an object as a constant does not make it immutable).

I can see situations in which, in addition to defining something like a constant, the programmer would want to make explicit that the object is immutable. But making it for every single constant looks overkill (not to say ugly) to me.

On the other hand, I don't like to diverge from main style guide. So I'm voting against the change, but in case of draw I could change my vote to break the tie.

jreidinger · 2016-07-22T09:02:06Z

one example where I have to disable it, as I need to modify constant with definining default handler - https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/stage1_proposal.rb#L256
but in general I find it useful as constants should be really constant to avoid surprise for people comming from other languages

wfeldt · 2016-07-22T09:02:22Z

This aims at changing the ruby language semantics. If and when ruby itself changes, that's another story. But here it's like using #define or __attribute__ to change C.

And sorry, saying that implementing a change now is ok because it can be easily undone later seems to be a strange kind of argument to me.

Moreover, it wouldn't be the first time that people used an anticipated future ruby feature in advance only to be hit by the slightly different real implementation in ruby later. So saying that you can painlessly remove the .freeze later and be done is not justified.

wfeldt · 2016-07-22T09:04:08Z

Really, don't we have anything better to do?

lslezak · 2016-07-22T13:50:52Z

OK, I have disabled the Style/MutableConstant cop check here and reverted the autocorrection done by Rubocop.

coveralls · 2016-07-22T13:52:19Z

Coverage increased (+0.2%) to 73.023% when pulling 0aa5cac on rubocop-update into f4f1fad on master.

ancorgs · 2016-07-22T13:56:04Z

lgtm

shundhammer · 2016-08-16T09:46:04Z

On 19.07.2016 13:49, Steffen Winterfeldt wrote:

lngtm, I stongly object to the .freeze.

+1

If this is needed, the interpreter should to it automatically. If it can't do
that, well, that's just too bad. But that's certainly not something the
developers should be bothered with.

Kind regards

Stefan Hundhammer shundhammer@suse.de
YaST Developer

SUSE Linux GmbH
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Maxfeldstr. 5, 90409 Nürnberg, Germany

jreidinger · 2016-08-16T10:03:14Z

@shundhammer interpret will do it automatic from ruby3, but now it is not done due to backward compatibility.

rubocop update

7348219

aschnell reviewed Jul 15, 2016
View reviewed changes

jreidinger added 2 commits July 18, 2016 09:12

simplify code

13bc9f6

disable zero predicate check

b268f06

more size empty reverts

2ec32dc

ancorgs reviewed Jul 19, 2016
View reviewed changes

remove unmatching enable

875f437

ancorgs reviewed Jul 19, 2016
View reviewed changes

lslezak added 2 commits July 22, 2016 15:48

Disable the Style/MutableConstant Rubocop check

4a26963

Revert the Rubocop Style/MutableConstant change

0aa5cac

lslezak merged commit 866069f into master Jul 22, 2016

lslezak deleted the rubocop-update branch July 22, 2016 13:57

rubocop update #83

rubocop update #83

Conversation

jreidinger commented Jul 15, 2016

coveralls commented Jul 15, 2016

aschnell Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lslezak commented Jul 15, 2016

aschnell commented Jul 15, 2016 • edited Loading

lslezak commented Jul 15, 2016

jreidinger commented Jul 15, 2016

aschnell commented Jul 15, 2016 • edited Loading

lslezak commented Jul 15, 2016

jreidinger commented Jul 15, 2016

aschnell commented Jul 15, 2016

lslezak commented Jul 15, 2016 • edited Loading

jreidinger commented Jul 18, 2016

coveralls commented Jul 18, 2016

aschnell commented Jul 18, 2016

jreidinger commented Jul 18, 2016

coveralls commented Jul 18, 2016

lslezak commented Jul 19, 2016

aschnell commented Jul 19, 2016

jreidinger commented Jul 19, 2016

wfeldt commented Jul 19, 2016

jreidinger commented Jul 19, 2016

jreidinger commented Jul 19, 2016

wfeldt commented Jul 19, 2016

wfeldt commented Jul 19, 2016

ancorgs commented Jul 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 19, 2016

lslezak commented Jul 20, 2016

lslezak commented Jul 20, 2016

wfeldt commented Jul 20, 2016

lslezak commented Jul 20, 2016

ancorgs commented Jul 21, 2016

wfeldt commented Jul 21, 2016

ancorgs commented Jul 22, 2016

lslezak commented Jul 22, 2016

lslezak commented Jul 22, 2016

ancorgs commented Jul 22, 2016

jreidinger commented Jul 22, 2016

wfeldt commented Jul 22, 2016

wfeldt commented Jul 22, 2016

lslezak commented Jul 22, 2016

coveralls commented Jul 22, 2016

ancorgs commented Jul 22, 2016

shundhammer commented Aug 16, 2016

Kind regards

jreidinger commented Aug 16, 2016

aschnell Jul 15, 2016 •

edited

Loading

aschnell commented Jul 15, 2016 •

edited

Loading

aschnell commented Jul 15, 2016 •

edited

Loading

lslezak commented Jul 15, 2016 •

edited

Loading