Introduce ContextDimension

[LecrisUT]

This is quite a big one, but I will keep the commits relatively logically contained for easier review commit by commit. The idea here is:

• Add a container for defining the context dimension comparison operators: ContextDimension
• Users can extend ContextDimension by sub-classing it and defining the _dimension_name it is linked to
• If a dimension is not found in the sub-classes, the create_default is used, which by default wraps around ContextValue for backwards-compatibility
• All of the comparator logic was moved to ContextDimension instead. Operators and decorators were added to manage these more dynamically
• Some cleanups
  • staticmethod -> classmethod to allow expanding the Context class
  • re_expression_triple was made a function for the reason above
  • Dropped some dead-code parts

Closes #289
Depends-on #297

---

Checklist

• implement the feature
• write the documentation
• extend the test coverage
• mention the version
• include a release note

[tcornell-bus]

blocked by #294

[thrix]

This does not look good:

❯ python3 -c 'import fmf; fmf.Context("module = perl:5.28").matches("module == perl")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
    import fmf; fmf.Context("module = perl:5.28").matches("module == perl")
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/var/home/thrix/git/github.com/teemtee/fmf/fmf/context.py", line 752, in matches
    result = self.evaluate(expression)
  File "/var/home/thrix/git/github.com/teemtee/fmf/fmf/context.py", line 805, in evaluate
    return self._op_core(dimension_name, values, operator)
           ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/home/thrix/git/github.com/teemtee/fmf/fmf/context.py", line 541, in _op_core
    assert isinstance(dimension_value, ContextDimension)
           ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

[thrix]

Case sensitivity is broken now

python - <<'PY'
from fmf.context import Context

context = Context(component="python3-3.8.5-5.fc32")
context.case_sensitive = False

print(context.matches("component == python3"))  # True
print(context.matches("component == PYTHON3"))  # Expected True, PR returns False
PY

True
False

[LecrisUT]

Yes, the tests show that. Please look at the current design and provide a suggestion. There are various courses of action here:

• Delete the case_sensitive option and tests
• Fail on unsupported case_sensitive option
• Deprecate case_sensitive and do some ugly workaround
• Maintain the current support

There is a broader discussion necessary on the support, e.g. in tmt we are mixing both case_sensitive = False in various adjust and the default in enabled_by_when for example.

[thrix]

I have no idea why the feature was added, I think we will need to find out first. But I would prefer first to provide a drop in replacement and work on dropping any functionality separately.

[LecrisUT]

Here are the facts I could gather:

• tmt is the only consumer of context other than fmf itself, and fmf cli does not expose context either. Let me know of instances that I did not look for, I considered artemis-pools, rh-kernel-stqe/python-stqe, and fmf-jinja
• case_sensitive was added in Allow case-insensitive matching of context dimension values #195, Handle all context dimension values case insensitive tmt#2357
• The case-sensitive handling in tmt is not consistent as seen in https://github.com/teemtee/tmt/blob/ee7b865b111c6f6d41114905431286ffd07ab807/tmt/steps/__init__.py#L2111-L2127

I personally don't see a reason to be conservative about this one since this change would need tmt change as well to be useful. There already is customizability and guidance if new users pop up. In the meantime, please review the design and the suggested way to opt-in the case-sensitivity! Right now the commits are very minimal and focused on the main functionality before it starts growing with the test adjustments and such.

@therazix @psss were involved in the introduction of the case-sensitive feature, so I will wait for their feedback and context in the meantime.

[psss]

Thanks much for preparing the draft of this. The overall design sounds good to me. There was a tangible use case behind introducing the case-insensitive comparison so we definitely want to keep the insensitive handling at least. By default, I'd vote for keeping the option to choose the behavior. Defining the sensitivity at the ContextDimension class level looks like a good approach to me. But, if this would complicate the implementation too much, I wouldn't be strongly against dropping case-sensitive altogether, especially if there are no clear consumers for it.

Before diving into all related detailed changes, could we outline how the is defined and is not defined operators could be handled? Would it be some special separate value-less class? That way sounds a bit complicated to me. If possible, I'd rather suggest to keep a single ContextDimension and somehow allow value-less handling inside it. @LecrisUT, do you see a way how we could implement the second path?

[LecrisUT]

By default, I'd vote for keeping the option to choose the behavior. Defining the sensitivity at the ContextDimension class level looks like a good approach to me.

That should still be available. The part that I would prefer to be dropped is the interface at the Context level and make ContextDimension the source of truth for that instead. I could make the current case_sensitive api still work, but it would be ugly and have potential race condition-like bugs.

Before diving into all related detailed changes, could we outline how the is defined and is not defined operators could be handled?

Good question, I do not have a good vision for those, here are some possibilities:

• Keep them at the Context level as special hard-coded values (current state)
• Extend the other 2 regex patterns with decorator and generalize them in Context
• Same as ☝️ but try to add it to ContextDimension (somehow) instead (quite difficult as I think about it, rather not)

Would it be some special separate value-less class?

Don't think we would need that. If needed I think we can just split the operators decorator and add them to a separate registrar (options 2 and 3 in above). The main thing here would be to generalize the Operators so that it can hook to other class.

[psss]

That should still be available. The part that I would prefer to be dropped is the interface at the Context level and make ContextDimension the source of truth for that instead. I could make the current case_sensitive api still work, but it would be ugly and have potential race condition-like bugs.

Sounds good to me to keep this at the ContextDimension level. Just to clarify on a tangible example: So if tmt would like to disable case sensitivity, it could inherit from DefaultContextDimension and change the class variable to False and handle all provided context dimensions with this custom class. No other change(s) needed?

If needed I think we can just split the operators decorator and add them to a separate registrar (options 2 and 3 in above).

Separate register for unary and binary operators sounds ok to me.

[LecrisUT]

So if tmt would like to disable case sensitivity, it could inherit from DefaultContextDimension and change the class variable to False and handle all provided context dimensions with this custom class. No other change(s) needed?

Exactly. I can update the tests to show this.

If needed I think we can just split the operators decorator and add them to a separate registrar (options 2 and 3 in above).

Separate register for unary and binary operators sounds ok to me.

👍

[psss]

So if tmt would like to disable case sensitivity, it could inherit from DefaultContextDimension and change the class variable to False and handle all provided context dimensions with this custom class. No other change(s) needed?

Exactly. I can update the tests to show this.

Good! 👍

[thrix]

This should now be _context_dimensions ?

[LecrisUT]

Sure, but I will wait until there is a review on the design instead of ping-ponging between implementations. Please provide feedback on the design as well

[happz]

LGTM, in general the operator-based approach fits my view how these things should be implemented. I guess I will have a comment or two, mostly minor ones, when I get back in the evening.