Add color and size tag support

[Dahie]

This is relating to #26

I opted to only have the options for :span and :remove for now. I'm not entirely happy about the use of global option, but I didn't want to rebuild the whole configuration management.

[jarrett]

Thanks! Would it be better if, in addition to the global config, we were to allow config overrides when #convert is called? I'd be happy to add that.

[Dahie]

That wouldn't actually solve my design issue: Since the Grammar parser is abstracted away with Parslet and we only define the Nodes the parser is using, I don't know how to pass data from RbbCode#convert to the node instances. This is where my global config comes in at 9b3dda5#diff-3c56d0b32c0d74f930a5a545082ee319R200

[jarrett]

I can pass the config options through. Would you like to be able to do the following?

RbbCode.new.convert('This` is [b]BBCode[/b]', output_format: :markdown, :unsupported_features: :span)

[Dahie]

I can pass the config options through.

Do you have an example?

The more I think about it, the more I want to avoid @@options. if we have to keep the @@options in RbbCode, the options are stored on class-level and all instances of RbbCode will access the same options hash. This may cause unintended effects and will be hard to debug for users.

Would you like to be able to do the following?

RbbCode.new.convert('This` is [b]BBCode[/b]', output_format: :markdown, :unsupported_features: :span)

Hm, I don't see how this would solve the @@options issue. Personally I don't see a gain in this right now. on the contrary, it'd break backwards-compatibilty for nothing. :/

[jarrett]

Example usage would be:

RbbCode.new.convert('This` is [b]BBCode[/b]', output_format: :markdown, :unsupported_features: :span)

So the question is whether you'd like to be able to call the API this way. That is, passing options on a per-convert basis, instead of only once upon instantiating RbbCode. I personally think it could be beneficial.

The class variable @@options doesn't currently exist, and I'm not planning on adding it. So I think we're in agreement on that. Or are you referring to the instance variable @options?

[Dahie]

I am introducing @@options here as a class variable in order to be able to use it in a static RbbCode.options called in the new Node classes. These don't have any reference to the RbbCode instance and I don't know how to inject options into them.

[Dahie]

RbbCode.new.convert('This` is [b]BBCode[/b]', output_format: :markdown, :unsupported_features: :span)

You can introduce it. Personally I'm don't need it and if it replaces the options via initializer everyone using the gem would have to change. So I'll not integrate here. :)

[jarrett]

No worries, nobody have to change their calls to the existing API. The per-convert options would be an override of the per-instance options. So the order of precedence would be: 1. Options set with `convert` (per-convert) 2. Options set with `new` (per-instance) 3. Default options (built-in)

[jarrett]

I updated the API on the master branch. As you'll see, this adds per-convert option overrides without breaking backwards compatibility. No class variable needed.

[Dahie]

Alright, I changed my branch to incorporate that. @@options is not removed.

[jarrett]

Sounds good. I’ll work on getting these merged into master.

On Thu, Jun 25, 2020 at 5:26 AM Daniel Senff ***@***.***> wrote: Alright, I changed my branch to incorporate that. @@options is not removed. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAD6UBWKHSDW4GYOV5JVYDRYMJ3ZANCNFSM4OEIXOFQ> .

-- -- This message, including attachments, is confidential and may contain privileged information. If you receive this message in error, please destroy it and notify me immediately.