questioning_authority icon indicating copy to clipboard operation
questioning_authority copied to clipboard

Clean Up TermController Logic

Open no-reply opened this issue 8 years ago • 3 comments

TermController has a bunch of logic for building authorities based on a largely undocumented namespacing pattern: https://github.com/projecthydra-labs/questioning_authority/blob/master/app/controllers/qa/terms_controller.rb#L35-L64

I haven't been able to completely make sense of everything that goes on here, a lot seems pinned to obscure specifics of various authority impls. We should replace the whole thing with a factory method an authority factory method and some kind of registry. I think it should be possible to accomplish this without breaking changes.

I have a branch that starts to work on this which fails a bunch of tests if you start to register authorities, various things break for reasons that are hard to pin down: https://github.com/projecthydra-labs/questioning_authority/tree/authority-factory

no-reply avatar Feb 22 '17 03:02 no-reply

Part of the complication seems to be that various authorities don't bother to raise NotImplementedError for #all, but I'm not sure why returning them from a factory method, rather than constantizing, makes any difference.

Several authorities fail miserably when altered to inherit from Authority::Base.

no-reply avatar Feb 22 '17 03:02 no-reply

@no-reply This was part of the original implementation where you could define your own authority as a constant like MyAuthority and the controller would register it for you by attempting to constantize it. Knowing what I know now, there are far better ways to do this. You're on the right track, I think, with your changes. init_authority should be removed from the controller completely and all it should do is consult the registry of authorities. If it's there proceed, if not, return a 501 or something.

I'm only vaguely familiar with the factory pattern, but if that can enforce the creation of new authorities, then I'm 👍 . The process of extending ::Base and raising "not implemented" errors was more a poor man's process. It does get you to write the methods you're supposed to, but there was never a documented process as to how to get to that point.

Feel free to ping me on any PRs or commits and I'll offer what feedback I can. Otherwise, I think any changes you're proposing regarding shoring up the API and codebase would be most welcome.

awead avatar Feb 22 '17 14:02 awead

I am not expecting this to be fixed in the next release of QA. I am leaving open in case we want to revisit this later.

elrayle avatar Mar 04 '19 16:03 elrayle