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
make the category param of gitpoller callable. #1432
Conversation
Probably you should give the whole changeDict there, like we do for the codebaseGenerator. |
ae6d27f
to
11a25f4
Compare
Cool idea! This needs docs, a release note, and tests. The tests should validate that the value passed to the callback is actually a changedict. |
11a25f4
to
ab9079e
Compare
What about defining a global callback like we do for the |
@delanne requested for review! |
We talked about passing the same "pre-change dictionary" to both codebaseGenerator and this callback, and validating that dictionary in the tests. |
ab9079e
to
cfb676f
Compare
=> needs work |
417df1c
to
753fae1
Compare
Don't we want to move the callback at the same level as the codebase generator and at the same time enabling the functionality for all changesources ??? |
if we want this functionality to all changesources, then we have to update all changesources (because it's not done in the BaseClass). Unfortunately, I don't have enought time to do that. |
Note: the code is not finished and not ready to be merged. |
I just mean: Why not implementing that in the base class ? |
753fae1
to
2bf6588
Compare
should be ok. |
Reviewers?! :) |
@@ -285,17 +288,21 @@ def _process_changes(self, newRev, branch): | |||
raise failures[0] | |||
|
|||
timestamp, author, files, comments = [r[1] for r in results] | |||
pre_change = self.master.config.preChangeGenerator(author=author, | |||
revision=unicode(rev), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would ascii2unicode
be more suitable here?
I agree with @benallard, I think this is a good improvement that needs to be in all the changesource code. This has few to do with gitpoller. I dont think this is a big work. You just have to put this code in the change data api. You move the test in the data api as well. I believe this will even simplify it. I don't get exactly the preChangeGenerator. |
The sticking point, for me, is this idea that we're building yet another callable that gets a partially-formed change dictionary, and I'm worried that we're going to see divergence in which keys are in that dictionary. If we can be consistent about that (in a testable way), then I'm OK with it. I agree that this isn't limited to GitPoller. |
40513fc
to
28175cd
Compare
any comments ? |
Sorry, I'll go through it later today (I did not notice there were more commits to review). |
- use the same pre_change dict as codebaseGenerator
- put the code in the DATA API: self.master.data.updates.addChange() accepts a callable for category
28175cd
to
83147f0
Compare
LGTM (provided we do not require implementing some common functionality in the base class) |
make the category param of gitpoller callable.
This PR is for review.
It makes the param category of gitpoller callable. the function takes only 1 arg: the branch.