Skip to content

Refactoring of all approvers and Approvals entry point #26

Description

@aneveux

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

Activity

self-assigned this
on Apr 19, 2018
added this to the 1.0 milestone on Apr 19, 2018

tyrcho commented on Apr 20, 2018

@tyrcho
Member

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 commented on Jun 30, 2018

@tyrcho
Member

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 commented on Jul 1, 2018

@tyrcho
Member

@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

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions