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

Save rule on enable/disable even if not selected. Closes #6163. #6183

Merged
merged 2 commits into from
Jan 25, 2017

Conversation

magao
Copy link
Contributor

@magao magao commented Jan 1, 2017

This pull request addresses issue #6163.

Correctly saves the enabled/disabled state of a rule when it is changed while the rule is not selected.

Note: this pull request includes a commit (ee0359c) which has been factored out to allow all the pull requests I am submitting to merge without conflicts.

Note that this pull request has been rebased on top of #6212 and uncrustified.

@sledgehammer999 sledgehammer999 added the RSS RSS-related issues/changes label Jan 18, 2017
@sledgehammer999
Copy link
Member

1st commit seems ok. 2nd has its own PR. The other uncrustifies other files.
I suspect you do the same for the rest of your PRs, so I want you to include commits that uncrustify the files that particular commit touches only. Thank you.

@magao
Copy link
Contributor Author

magao commented Jan 18, 2017

A lot of commits/PRs touch the same files, which is why I created a single uncrustify PR. For those cases, how do you want me to handle them?

  1. Have an uncrustify commit per PR (which in many cases will be identical to a commit in other PRs)

  2. Have one or more uncrustify commits so each PR is on top of only uncrustify commits that affect that PR (but multiple PRs may be on top of the same uncrustify commits)?

@sledgehammer999
Copy link
Member

Put the rss related uncrustify commit into one of your multiple rss PRs. Then tag me so I can review+merge it. The rest of your PRs won't need that commit anymore.
PS: One PR of yours touched non-rss related issues so that's why I didn't want to merge as-is.

And for future reference: If you must uncrustify, then do it in the same commit. And uncrustify only the files you want to touch.

@magao
Copy link
Contributor Author

magao commented Jan 18, 2017

The reason I did the uncrustifying in a separate commit was that it allowed all the PRs to merge cleanly no matter what order they were merged. Only doing the uncrustifying in the commit that included the change meant they didn't. Also, in many cases the uncrustifying dominated and the actual changes were hard to see as a result.

@sledgehammer999
Copy link
Member

I understand. That's why I tell you to include the uncrustify changes to only one for current PRs. Choose a simple one that I didn't find any faults. Then after I merge it the other PRs should be ok or need just a simple rebase.

@magao
Copy link
Contributor Author

magao commented Jan 18, 2017

In that case you could just merge #2192 #6212 which is a separate PR just for the uncrustify (and all other PRs are on top of).

@sledgehammer999
Copy link
Member

As I have already told you it touches unrelated files. Specifically it touches changes to mainwindow.{cpp|h} which I don't want to merge right now.

@magao
Copy link
Contributor Author

magao commented Jan 20, 2017

A forum is such a bad way to work this out - if we were talking together I'm sure we'd have this hashed out in 5 minutes.

So if I just removed the changes to mainwindow.h/cpp from #2192 #6212 would it be OK? Or do you want me to collapse/squash the rss-related uncrustifying into one of the PR commits you've otherwise approved (which means the commit will contain a pile of irrelevant changes)? Or delete the #2192 #6212 PR and just include the commit (minus the mainwindow changes) in one of the other PRs?

And for commit that changes mainwindow.h/cpp (currently fa6cdbd but would change after history editing) I shouldn't uncrustify the files but just make sure my changes follow current coding style?

@sledgehammer999
Copy link
Member

So if I just removed the changes to mainwindow.h/cpp from #2192 would it be OK?

Yes. I'll check it in a few minutes and then merge.
For future reference, uncrustifying commits should be a separate commit but belong to the same PR they reference and only touching files you touch with the non-uncrustifying commit.

@magao
Copy link
Contributor Author

magao commented Jan 20, 2017

Give me half an hour? Still waiting for caffeine to kick in (8am Saturday morning ;)

@sledgehammer999
Copy link
Member

Wait! You have confused me. #2192 is not YOUR PR. #6112 is.

@sledgehammer999
Copy link
Member

sledgehammer999 commented Jan 20, 2017

And for commit that changes mainwindow.h/cpp (currently fa6cdbd but would change after history editing) I shouldn't uncrustify the files but just make sure my changes follow current coding style?

You should uncrustify with new commit into the relevant PR.
So you either uncrustify first and then do your changes.
Or do your changes following the old style, and then uncrustify.
(this may not be ideal but I won't bitch about it)Or do your changes following the new style and then uncrustify.

@magao
Copy link
Contributor Author

magao commented Jan 20, 2017

Yes - I'm an idiot. #6212 (#2192 is the issue). I've pushed the new commit there.

@magao
Copy link
Contributor Author

magao commented Jan 22, 2017

@sledgehammer999 Rebased on top of master.

@sledgehammer999
Copy link
Member

Thx

@sledgehammer999 sledgehammer999 merged commit 5df8ad3 into qbittorrent:master Jan 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RSS RSS-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants