Group by keyword

[mrkaiser]

• Added feature to group by keyword in the step glossary. -T is the argument
• Added an associated test

This allows people to know what steps are used by what keywords

[bradsbrown]

@mrkaiser looks like the checks had some failures as well — be sure to review and correct those as you prepare new commits.

[mrkaiser]

Fixed the issues (I think, I'm waiting on the build). Thank you for your comments.

One thing that may help as a complete outsider is a CONTRIBUTING.md. The env setup isn't super familiar to me and I had to workaround potentially broken versions on my local because of build issues (e.g typed_ast seems to be completely broken for compiliation with python 3.11 and since by default poetry looks at system even if pyenv is available this breaks). Appreciate everything, hopefully my comment was helpful as well. Thanks!

[bradsbrown]

@mrkaiser looks like the one check you have left to hit is the version bump check — just a poetry version patch should fix you up!

[mrkaiser]

@bradsbrown I believe I have addressed all comments and the checks have passed.

[bradsbrown]

@mrkaiser Yep, looks great! I've added requests to get an additional review or two before merge.

[dgou]

Thanks for the enhacement @mrkaiser Let me know if you want to take a shot at the refactoring. I can let you know my thoughts, but I would like to hear yours before having any undue influence explaining mine.

[mrkaiser]

@dgou

Hi!

Thank you for all your thoughtful comments!

Couple of thoughts:

• If there were multiple ways, I would prefer to have enums bound to functions that I could pass in. If a second use case comes through. I think that makes sense, so that a single "group by" could just take a group by func. Personally I'm a fan of higher kinded functions and try to utilize them when possible.
• I actually have tried to split it out. The key issue though is the if group_by. Eventually that if statement lives somewhere, even if it's early when doing the argparse. Now you could continue and split it into two functions, say make_steps_plain and make_steps_group_by. But you'd have to reasonably make an additional helper because there is shared functionality in the preamble and the actual step building. The total code ends up a bit longer about 10 or so lines (which is about 10% of the total lines in the file). Now I have moved the main step building, and am sharing that code between both non and grouped by. Overall I understand the fear of bools. If they product vastly different functions and you're hiding that with a "simple boolean", that is probably going to be hard to reason about later. I'm not entirely sure that's happening here, but would love to discuss further.
• The original if above the function for glossary writing was

  if not step_glossary

  My thought process here was add to the if with the data structure I needed. Now we could collapse to 1 single data structure and then just get the flattened first. But that may be more logically complicated than what's here. But am happy to explore other ideas as well.

[dgou]

@mrkaiser Thanks for the response. It's been long enough now that I will have to sit with the code and get my brain to where it was from my previous comments. Will try to do that tonight, if not the next few nights. Thanks!

[mrkaiser]

No rush at all. I won't be able to look until this weekend anyways. Appreciate any thoughts, really open to suggestions here. Overall I agree that booleans can be smells and try to avoid them. In this case though I just wanted to make sure I didn't over index on that and end up making something that may end up harder to reason about.

[dgou]

Yeah, I thought I would get to it this week, but it looks like it'll be the weekend for me too.