RM-43-records_mover-db-mysql-load_options.py-113-1-C901-mysql_load_options-is-too-complex-18

[ryantimjohn]

No description provided.

[Brunope]

you should also mention that on success, this removes 'encoding' from the load parameters unhandled_hints. its a little weird to do modifications in a "getter" but eh

[ryantimjohn]

Very good point; I'm open to another name. Additionally, PEP conventions suggest to skip get_ names entirely.

Any ideas for prepends? Maybe it'd just be better to skip them.

[Brunope]

I think the factoring of logic into process_ functions is fine, as is the load_params construct.

I don't love the fact that various get_ methods are also performing modifications on the hints via quiet_remove, maybe the removal can be lifted up a level to the caller after calling process_?

I'm also not a fan of moving these functions, especially the process_x ones, into a "utilities" file. They aren't shared functions to be used generically, they are very specific to mysql_load_options. I'd rather everything related live in one place, so I don't have to jump around to navigate, especially since it's a relatively simple sequence of process one thing, then process this next thing, then process the last thing, etc.

[ryantimjohn]

I think the factoring of logic into process_ functions is fine, as is the load_params construct.

I don't love the fact that various get_ methods are also performing modifications on the hints via quiet_remove, maybe the removal can be lifted up a level to the caller after calling process_?

I'm also not a fan of moving these functions, especially the process_x ones, into a "utilities" file. They aren't shared functions to be used generically, they are very specific to mysql_load_options. I'd rather everything related live in one place, so I don't have to jump around to navigate, especially since it's a relatively simple sequence of process one thing, then process this next thing, then process the last thing, etc.

This all makes sense. If we want to do this, I do want to keep the docstrings so we're going to have to either (in order of preference) 1) make the bigfiles ignore comments or 2) get bigfiles to ignore this file entirely. I'm going to add a ticket to our board for that.

[Brunope]

When I was looking for how to get bigfiles to ignore setup.py, I saw that there are some files excluded in Rakefile.quality. But I believe this excludes the files from ALL quality checks, not just bigfiles, which I don't know if we want.

Edit: I do think the docstrings are great and worth keeping as well.

[ryantimjohn]

When I was looking for how to get bigfiles to ignore setup.py, I saw that there are some files excluded in Rakefile.quality. But I believe this excludes the files from ALL quality checks, not just bigfiles, which I don't know if we want.

Sounds like we might be coming around to the idea that it's just worth getting rid of the bigfiles check

[ryantimjohn]

@Brunope seems like we might actually just be able to add a tag to the bigfiles invocation:
https://github.com/apiology/bigfiles

I'm going to try this out

[ryantimjohn]

Oh I see, we just invoke quality without actually specifying bigfiles. Damn.

[ryantimjohn]

Alright @Brunope this is now ready for review.

[Brunope]

looks great!