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

Refactoring of all approvers and Approvals entry point #26

Open
2 of 9 tasks
aneveux opened this issue Apr 19, 2018 · 3 comments
Open
2 of 9 tasks

Refactoring of all approvers and Approvals entry point #26

aneveux opened this issue Apr 19, 2018 · 3 comments
Assignees
Milestone

Comments

@aneveux
Copy link
Member

aneveux commented Apr 19, 2018

The Approvals object and all the Approver objects are to be refactored.

Here are some thoughts after reading again the source code:

  • Documentation should be updated to state that Approvals is an easy entry point for accessing only default configuration,
  • Approvers can be used for advanced configuration of your tests and validations,
  • The custom extension should disappear. Using the csv extension might sound appealing (because of opening the files in other editors), but it actually is really confusing and might lead to other issues (ignoring csv in SCMs, etc.) - I can elaborate on that topic, but my opinion is: we can format the file as if it is a csv file, but it needs to remain a .approved file,
  • The custom file name isn't a feature we planned yet, and there is more to think about it than just an entry in the builder object, so it should disappear as well (I'm in favor of an agile approach for that: we implement if and only if we need it (on a real use case))
  • As far as possible, the source code itself should allow to understand that Approvers are actually just advanced entry points for approvals. Maybe putting them in an advanced package could do the trick?
  • Global refactoring (variable names, functions, etc.)
  • Javadoc the whole thing
  • Review the error messages and align them to all the other error messages
  • Remove from the ApprovalFiles class all the things linked to the extensions
@aneveux aneveux self-assigned this Apr 19, 2018
@aneveux aneveux added this to the 1.0 milestone Apr 19, 2018
@tyrcho
Copy link
Member

tyrcho commented Apr 20, 2018

Agreed !

I would add

  • remove code duplication in configuration of different children of Approvers, maybe with a proper "config" object, this code scales really bad
  • think about using composition rather than inheritance so the current children of Approver use an instance of it rather than inherit

@tyrcho
Copy link
Member

tyrcho commented Jun 30, 2018

1st step for the refactoring in edfc1ae
We should eventually be able to get rid of FolderApprover.

Note : IDEA diff command works well with folders as arguments, as well as some external diff programs :D

@tyrcho
Copy link
Member

tyrcho commented Jul 1, 2018

@aneveux I did a few refactorings with what bothered me the most, what do you think ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants