Closed bentorkington closed 2 years ago
I don’t know what a draft PR is, I’ll have to have a look at the GitHub docs. Happy to switch it on if that makes the workflow easier.
It looks like it's not even a thing on Github, sorry. It was just an option in GitKraken I thought I'd try and the error was 'this repo doesn't support draft PRs' where it should probably not present the option for a Github repo
I’m pretty happy with the way this is coming along. I can see a few small questions coming up with consistent naming conventions around use of ‘filter’ and ‘modifier’ but this is all stuff caused by me, not anything you’ve done.
Maybe also some questions around the structure of the namespace and organisation of the classes but overall I think this is very close to how I envisaged it working. I’ll work through in more detail later tonight.
I'm starting to see some shortcoming in my design of IStringModifier here. I tried to keep it simple by having the base Modify()
method only require a string, to allow injection of not only the Options
object, but any other parameters that could customise it, [as in my StudlyCaps example here] (https://github.com/fictiveworks/CalyxSharp/issues/13#issuecomment-1267523803). In doing so, I probably overcooked the whole idea, and there's now way for users of the Grammar class to access the rsulting Options
object anyway 🫢
I think it’s fine to assume all these filters/modifiers (I need to get the language more precise here) are stateless functions and need to define all their dependencies as part of their input args signature.
Okay. I'll axe this overcomplicated StringModifier class and interface, stick to calling them 'filters' in the code that remains, and they can go back to being static methods with attributes to name them. We'll need to handle the case where someone annotates an incorrect method signature with a FilterNameAttribute, that could either be caught when the filter is invoked, or when the custom attributes are added to the registry.
I'm also a bit hung up on the titlecase and sentencecase implementations. I'm disappointed CultureInfo.ToTitleCase is really just 'initial caps on every word' and haven't been able to find good implementations to use for inspiration yet.
just checking we're on the same page here:
need to define all their dependencies as part of their input args signature.
that implies the input args signature is strictly (string input, Options opts)
, right? Because as far as I can see there's no other way to do that without making them stateful objects from some sort of factory.
I’m not super concerned about the naming at this stage, as can always do another pass later and figure that out from the big picture API down to the internal API and contracts. I think I need to finish writing the docs for this which will hopefully help things make sense and fall into place a bit better.
Will continue this on #26
WIP of filter modifiers for review. I tried to submit this as a draft but apparently this repo doesn't support draft PRs, so I guess we'll just abandon and redo it?
I would have liked to push as several smaller commits but it's all codependent so they can't be split and each one still build individually.
Still todo: