-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Refactoring for additional feature handling #39
Comments
Hi @johnharris85, Good to hear from you again - when you have time head over the jaas project again and checkout the latest features we've been adding. Full disclosure - I'm unlikely to take refactoring changes before they are necessary for a feature / approved request. The highest priority tickets are marked with a label: priority/high #17 is marked as low priority, but needs agreement on how it should work and be configured before any work is done on it. I've added some more context there for review |
Thanks @alexellis! Ye wanted to ask here about refactoring before doing anything for that very reason :) Right now it looks like the codebase assumes only one 'feature' for PRs, so that function may turn into a long list of What's your view on requiring refactoring to support new 'features' (in addition to dco checking, as defined in the config yaml)? |
Simplest approach first, duplicate some statements when necessary, then review and if it makes sense after that introduce new abstractions. |
Since this is not an issue I'm going to close it but you can keep commenting. |
John would you like to work on this? #42 also I can't see you on our Slack community.. would you join to coordinate work? email [email protected] |
Awesome project! Was thinking about taking a crack at #17 and wandered how you wanted to handle more features being added in the code? Checks like this one assume only one feature for handling PRs, and this might need to be refactored if more are being added?
Also the code in handlePullRequest may need some refactoring to run through different features if they're enabled / disabled etc...
#17 is just a single addition so can fit into the current model, but just wanted to check on direction before I do anything, or if you think refactoring for potentially more scope later on is the way to go first?
Happy to have a go at either, thanks!
The text was updated successfully, but these errors were encountered: