Recursive Tree initialization. Why static method?

[abitrolly]

There is Tree.init() static method that way too confusing.

tmt/tmt/base.py

Lines 3075 to 3076 in f4a0480

	@staticmethod
	def init(

It creates another instance of Tree down the line.

tmt/tmt/base.py

Line 3090 in f4a0480

tree = Tree(logger=logger, path=path)

And then sometime even Tree with other arguments.

tmt/tmt/base.py

Line 3110 in f4a0480

fmf.Tree.init(path)

But this base.py Tree.init() contains very user specific and high level logic, like committing paths to .git repository. I expect this to be a standalone function, either in base.py or in some command file. Like factor out these two lines into actual command logic.

tmt/tmt/cli.py

Lines 1620 to 1621 in f4a0480

	tmt.Tree.store_cli_invocation(context)
	tmt.Tree.init(logger=context.obj.logger, path=Path(path), template=template, force=force)

The Tree even has some helpers to store cli invokation context? What? It looks like a God Class (https://linearb.io/blog/what-is-a-god-class).

So, how about moving this stuff out? At least the init method?

[happz]

There is Tree.init() static method that way too confusing.

tmt/tmt/base.py

Lines 3075 to 3076 in f4a0480

	@staticmethod
	def init(

It creates another instance of Tree down the line.

tmt/tmt/base.py

Line 3090 in f4a0480

tree = Tree(logger=logger, path=path)

And then sometime even Tree with other arguments.

tmt/tmt/base.py

Line 3110 in f4a0480

fmf.Tree.init(path)

I can imagine moving some of the functionality into fmf library, e.g. the block checking whether a tree already exists, or whether we're creating a nested tree, could be useful in general, not just in tmt. The method could be simplified for sure.

But this base.py Tree.init() contains very user specific and high level logic, like committing paths to .git repository. I expect this to be a standalone function, either in base.py or in some command file. Like factor out these two lines into actual command logic.

tmt/tmt/cli.py

Lines 1620 to 1621 in f4a0480

	tmt.Tree.store_cli_invocation(context)
	tmt.Tree.init(logger=context.obj.logger, path=Path(path), template=template, force=force)

The Tree even has some helpers to store cli invokation context? What?

Yes, it does, like many other classes derived from tmt.utils.Core. In tmt, CLI options are attached to corresponding classes to be later inspected (test -n foo will attach options to tmt.base.Test class, for example). So this is probably not going to be removed. On the other hand, I'm not sure whether init actually needs to invoke this operation, there is dry = Tree._opt('dry') but that could be converted to kwarg, IMHO.

It looks like a God Class (https://linearb.io/blog/what-is-a-god-class).

So, how about moving this stuff out? At least the init method?

While I don't mind simplifying Tree.init, refactoring it, and moving things out of it, I am less sure about moving the whole method outside of Tree class. To me, the idea of Tree itself knowing how to initialize its own on-disk structures seems correct, supporting encapsulation. But that ends around line 3117 - I can easily imagine the "commit the .fmf metadata directory into git if possible" would be refactored outside of this method and called by the command, the same for creating plan/test/story from a template.

[abitrolly]

So what is the separation of concerns between tmt.Tree and fmf.Tree?

Why Tree() is not enough and Tree.init() is needed?

[happz]

So what is the separation of concerns between tmt.Tree and fmf.Tree?

fmf.Tree takes care of loading and composing fmf tree from files and directories. tmt.Tree is then instantiated on top of this data structure, and adds tmt-specific semantics to fmf objects - see tests(), plans(), stories(). tmt.Tree asks fmf.Tree to search for objects according to various criteria, feeding them back to other parts of tmt. tmt.Tree never walks directories, never reads files, never evaluates adjust rules.

Why Tree() is not enough and Tree.init() is needed?

tmt.Tree() instantiates new Tree instances from existing fmf tree, tmt.Tree.init() initializes new fmf tree in a given path. The former happens way more often when tmt works with existing metadata trees (tmt run, tmt ls, ...), the latter only from time to time, when a user runs tmt init.

[abitrolly]

So fmf operates with file and directory structure. Then tmt adds tests, plans and stories? But tests, plans and stories are part of FMF specification https://tmt.readthedocs.io/en/stable/spec.html so I would say the separation of concerns is broken here.

tmt.Tree() instantiates new Tree instances from existing fmf tree, tmt.Tree.init() initializes new fmf tree in a given path.

It is really complicated. Everything is a Tree, but as I see it, there is at least FSTree with file operations, etc. and Metadata object, which is nested structure that could be serialized (saved) into filesystem.

I am not sure why there are multiple Metadata objects for each operation (tmt run, tmt ls). Metadata in the specification doesn't include user commands like run, so maybe the God object is too coupled.

In any case, it is just my 0.02 observations. Not something I can fix. tmt is a useful tool for running tests in isolation, but it carries complicated burden of concepts and, as a result, overcomplicated user interface with steep learning curve and degraded user experience.

[happz]

So fmf operates with file and directory structure. Then tmt adds tests, plans and stories?

Correct.

But tests, plans and stories are part of FMF specification https://tmt.readthedocs.io/en/stable/spec.html so I would say the separation of concerns is broken here.

Incorrect: tests, plans, and stories are not part of FMF specification. As written on the referenced page:

Flexible Metadata Format is used to store data in a concise human and machine readable way plus adds a couple of nice features like virtual hierarchy, inheritance and elasticity to minimize data duplication and maintenance.

fmf is nothing but an extension of YAML. Defines how keys and values should stored in files to take advantage of features fmf provides - inheritance, elasticity, adjust, and so on - but on its own, fmf defines no keys, except adjust. A fancy YAML parser.

The fact that a test may have a duration key, what might be its value, and what would be its effect, or even the fact that some keys together represent a test, that's part of tmt specification. fmf does not care a single bit whether it's duration or timeout. Define how duration and other test fields should be written in a markdown file and we'd be in the same situation, neither fmf nor markdown own or define test, plan, or stories' keys.

tmt.Tree() instantiates new Tree instances from existing fmf tree, tmt.Tree.init() initializes new fmf tree in a given path.

It is really complicated. Everything is a Tree, but as I see it, there is at least FSTree with file operations, etc. and Metadata object, which is nested structure that could be serialized (saved) into filesystem.

I suppose this is where the confusion lies. You may think of fmf.Tree as something json.load() would return, with some extra added functionality and extra methods. This is where the border is: fmf.Tree is what fmf package loaded from directories and files, just like a YAML or JSON or TOML parser would if coupled with some os.walk() wisdom, tmt.Tree gives this content meaning.

fmf.Tree represents raw data, tmt.Tree is aware of the fact that some nodes in this tree are plans and some are tests and how to tell which is which. fmf is a generic library, absolutely unaware of tmt, just like ruamel.yaml is unaware of how its users treat keys and values loaded from YAML files.

I am not sure why there are multiple Metadata objects for each operation (tmt run, tmt ls). Metadata in the specification doesn't include user commands like run, so maybe the God object is too coupled.

I'm not sure I understand this bit.

In any case, it is just my 0.02 observations. Not something I can fix. tmt is a useful tool for running tests in isolation, but it carries complicated burden of concepts and, as a result, overcomplicated user interface with steep learning curve and degraded user experience.

I'm sorry to hear that, I believe we are always open to suggestions and improvements.