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

make the category param of gitpoller callable. #1432

Merged
merged 6 commits into from Jan 23, 2015

Conversation

delanne
Copy link
Contributor

@delanne delanne commented Dec 9, 2014

This PR is for review.
It makes the param category of gitpoller callable. the function takes only 1 arg: the branch.

@benallard
Copy link
Contributor

Probably you should give the whole changeDict there, like we do for the codebaseGenerator.

@djmitche
Copy link
Member

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. buildbot.test.util.validation can help with that (and remember, this is a data API changedict, not DB API)

@benallard
Copy link
Contributor

What about defining a global callback like we do for the codebaseGenerator ? This way, it will be available to all change sources at once !

@sa2ajj
Copy link
Contributor

sa2ajj commented Dec 11, 2014

@delanne requested for review!

@djmitche
Copy link
Member

We talked about passing the same "pre-change dictionary" to both codebaseGenerator and this callback, and validating that dictionary in the tests.

@delanne
Copy link
Contributor Author

delanne commented Dec 12, 2014

=> needs work

@benallard
Copy link
Contributor

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 ???

@delanne
Copy link
Contributor Author

delanne commented Dec 16, 2014

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.

@delanne
Copy link
Contributor Author

delanne commented Dec 16, 2014

Note: the code is not finished and not ready to be merged.

@benallard
Copy link
Contributor

I just mean: Why not implementing that in the base class ?

@delanne
Copy link
Contributor Author

delanne commented Dec 17, 2014

should be ok.

@sa2ajj sa2ajj removed the needs work label Dec 17, 2014
@sa2ajj
Copy link
Contributor

sa2ajj commented Dec 17, 2014

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),
Copy link
Contributor

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?

@tardyp
Copy link
Member

tardyp commented Dec 29, 2014

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.
self.master.data.updates.addChange()
This api would accept a callable for category.

You move the test in the data api as well. I believe this will even simplify it.

I don't get exactly the preChangeGenerator.

@djmitche
Copy link
Member

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.

@delanne delanne force-pushed the CallableCategory branch 2 times, most recently from 40513fc to 28175cd Compare January 5, 2015 14:38
@delanne
Copy link
Contributor Author

delanne commented Jan 19, 2015

any comments ?

@sa2ajj
Copy link
Contributor

sa2ajj commented Jan 19, 2015

Sorry, I'll go through it later today (I did not notice there were more commits to review).

@sa2ajj sa2ajj removed the needs work label Jan 19, 2015
Xavier Delannoy and others added 6 commits January 20, 2015 09:30
 - 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
@sa2ajj
Copy link
Contributor

sa2ajj commented Jan 20, 2015

LGTM (provided we do not require implementing some common functionality in the base class)

sa2ajj pushed a commit that referenced this pull request Jan 23, 2015
make the category param of gitpoller callable.
@sa2ajj sa2ajj merged commit 1e39efe into buildbot:master Jan 23, 2015
@delanne delanne deleted the CallableCategory branch October 20, 2015 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants