[Final] Fix Empty unit spinners and spin button

[chimosky]

Activity runs fine but i get this error """Traceback (most recent call last): File "/home/ibiam/Activities/convert/activity.py", line 207, in _call self._update_label() File "/home/ibiam/Activities/convert/activity.py", line 195, in _update_label new_convert = locale.format(fmt, float(convert_value)) ValueError: could not convert string to float: None """

[quozl]

I'll wait until you've fixed that error then, and mark the pull request as in progress. Wouldn't want to merge a commit with a message about a problem unrelated to the commit.

[chimosky]

Done.

[rhl-bthr]

Reviewed and Tested.

• [ ] PyGI warnings logged
• [ ] Every item in the combobox is displayed twice (side by side)

[chimosky]

• [ ] Every item in the combobox is displayed twice(side by side)

One for the unit being converted from, the other the unit being converted to.

[rhl-bthr]

Sorry, my statement might have had been ambiguous, This is what I meant:

[chimosky]

Just noticed it too.

[chimosky]

Change made

• [x] Every item in the combobox is displayed once.

[chimosky]

@quozl Please review

[quozl]

Quick review; did not understand 565e3b89e2, any more detail?

[chimosky]

If active < 0 returns None, the value of text = keys[active] is also None so i thought of setting active to 0 instead of returning None since _get_active_text returns None which can't be converted to float and it worked -- i think it also works for none-negative values --.

[chimosky]

I also just noticed that without the active check, the activity runs fine. So I've removed the check.

[chimosky]

@quozl Kindly review.

[quozl]

Thanks for the update.

If active < 0 returns None, the value of text = keys[active] is also None so i thought of setting active to 0 instead of returning None since _get_active_text returns None which can't be converted to float and it worked -- i think it also works for none-negative values --.

Okay, thanks. This should be in the commit message; because pull request discussions are not archived, only the commit message is kept. If after reading below you think it should still be that way, please use git rebase -i or other methods to rewrite your commit message.

I also just noticed that without the active check, the activity runs fine. So I've removed the check.

Respectfully disagree.

The _get_active_text method takes as input a Gtk.ComboBox instance, and returns the text corresponding to the currently active item. It does this using Gtk.ComboBox.get_active method. Documentation says the method may return -1 when there is no active item. An index of -1 will cause an error in the line;

text = keys[active]

Because there is no key -1 in the dictionary.

The check for less than zero precedes this line, in the master branch, and you've removed it; perhaps you did not fully test the activity? Even if the code takes some other action to ensure there is an active item, there may be some way to not have an active item.

[chimosky]

Thanks, I'll rebase and send a new PR.

[quozl]

A new pull request is irritating, because we lose the discussion. Please just rebase properly and force push. If you don't yet know how to rebase, it's about time you learned by experience.

[chimosky]

Done, commit message not edited though.

[quozl]

Tested. Now I wait for you to finish the job;

• [ ] rebase to squash the commits and rewrite the commit messages,
• [ ] force push.

Then I shall review your commit message against your source code changes. What is merged should be the minimal number of commits for each problem fixed.

Don't use your commit message to explain the exact change; we can see that from the commit diff. See our guide to making commits.

[chimosky]

I've tried rebasing but it's not working for me, is this okay?

[quozl]

Thanks, that's progress. You've rebased 0a650e221e628d80aabc143d5e0464bfb15063d5 to 65ca39667e8603cb14d70481963ac03797783aaf with no other change except commits and commit messages.

Now I've read the commit in detail and used the guide for reviewers. As you read this, also have 65ca396 open in another window or tab, so you can see what I see.

• detect trivial mistakes,

  • [ ] your commit removes a blank line from the standard license heading; please don't,
  • [ ] your commit message is not properly wrapped and spaced.

• maintain consistent and good code quality,

  • looks consistent and good.

• reproduce test results, (especially for critical repositories),

  • speaking for Sugar Labs; this not a critical repository, and speaking for OLPC, we don't ship this activity yet, but I'm thinking about it,
  • skipped test, as already did that with 0a650e221e628d80aabc143d5e0464bfb15063d5, and 65ca396 is functionally identical.

• maintain a useful git commit history for use by git bisect, and developers who read it,

  • [ ] fail; your commit message does not say what the problem was, how it happened, or what was done to fix it; in particular it fixes https://github.com/sugarlabs/convert/issues/7 but does not say so; it does not say "fix empty unit spinners",
  • [ ] fail; your commit removes ENTER_KEY, but doesn't explain why in the commit message; my guess is you removed it because it is unused, but as it has nothing to do with solving the problem your removing it looks a bit mysterious,
  • [ ] fail; your commit changes which Gtk.ListStore is given to a Gtk.ComboBox, but doesn't explain why in the commit message; my guess is that it was a harmless mistake in the code; harmless because the two Gtk.ListStore instances are identical,
  • [ ] fail; your commit calls set_numeric on the Gtk.SpinButton, but doesn't explain why in the commit message, see also https://github.com/sugarlabs/convert/issues/14,
  • [ ] fail; your commit removes a call to add_attribute for one of the Gtk.ComboBox but not the other, but doesn't explain why in the commit message; I can't imagine why, see also https://github.com/sugarlabs/convert/issues/11,
  • [ ] fail; your commit message mentions a problem that does not occur in master; a problem that you had midway through your changes,

• maintain other records, such as issues, tickets, and documentation,

  • is fine,

• not waste the time of the contributor, by doing anything trivial that otherwise the contributor might have to do.

  • reviewer could rewrite the commit message,
  • reviewer could split the patch by problem fixed,
  • reviewer could remove the apparently incorrect changes.

So there's my quandry. I could follow my own advice in guide for reviewers to rewrite, rebase and split your patch, something which will take me no more than five minutes. But I'm not sure if that's what you intend, it takes agency from you, and you won't have tested what I commit.

You've shown such uncommon resistance to rewriting patches and writing commit messages that I'm worried you are actively avoiding learning these skills, and I don't want you to miss out on the opportunity to do so.

If you have problems with rebasing, you might mention what they are.

[chimosky]

Changes made

• [x] your commit removes a blank line from the standard license heading; please don't,
• [x] your commit message is not properly wrapped and spaced.
• [x] fail; your commit message does not say what the problem was, how it happened, or what was done to fix it; in particular it fixes #7 but does not say so; it does not say "fix empty unit spinners",
• [x] fail; your commit removes ENTER_KEY, but doesn't explain why in the commit message; my guess is you removed it because it is unused, but as it has nothing to do with solving the problem your removing it looks a bit mysterious,
• [x] fail; your commit changes which Gtk.ListStore is given to a Gtk.ComboBox, but doesn't explain why in the commit message; my guess is that it was a harmless mistake in the code; harmless because the two Gtk.ListStore instances are identical,
• [x] fail; your commit calls set_numeric on the Gtk.SpinButton, but doesn't explain why in the commit message, see also #14, looking right now
• [x] fail; your commit removes a call to add_attribute for one of the Gtk.ComboBox but not the other, but doesn't explain why in the commit message; I can't imagine why, see also #11,

Some of the above have been put into the same commits, where this occurs the commit message clearly states it.

You've shown such uncommon resistance to rewriting patches and writing commit messages that I'm worried you are actively avoiding learning these skills, and I don't want you to miss out on the opportunity to do so.

Thanks, I'm learning those skills and I promise not to miss out on this oppurtunity.

If you have problems with rebasing, you might mention what they are.

I hope you don't mind me sending a personal email.