Item#references: initial support of many ref items (Take 3)

[stanislaw]

I am opening this as a separate MR again because the previous one received too many comments.

I really need some collaboration from the maintainers because the scope is getting quite big. Unfortunately, I don't see anything meaningful that I could split out to become separate MRs.

Here are major comments:

1. This MR has optional keyword implemented as was suggested by @sebhub 👍

The keyword could be added to file type references in addition to the lines and stamp support. If keyword is present, then the keyword must be contained in the file content.

2. I don't know how to fit the full references into the Python GUI given that we also want to track the optional keyword there. How many people actually use the Python GUI and how much should we care about 100% match between the command-line and GUI versions?

3. Do we really need a type: file part or just having path is enough? Can we imagine anything else except references to single files? I am quite convinced that referencing folders is not practical and should always be replaced by referencing single files.

 references:
- path: tests/test1.cpp
  type: file
  keyword: REQ1
- path: tests/test2.cpp
  type: file
  keyword: REQ2

4. I am seriously suspicious about my Python writing 😅 I am happy to learn from you about every good practice that I might be missing.

[sebhub]

I am opening this as a separate MR again because the previous one received too many comments.

I really need some collaboration from the maintainers because the scope is getting quite big. Unfortunately, I don't see anything meaningful that I could split out to become separate MRs.

Here are major comments:

1. This MR has optional `keyword` implemented as was suggested by @sebhub +1

The keyword could be added to file type references in addition to the lines and stamp support. If keyword is present, then the keyword must be contained in the file content.

I would generalize this to regex.

[...]

2. Do we really need a `type: file` part or just having `path` is enough? Can we imagine anything else except references to single files? I am quite convinced that referencing `folder`s is not practical and should always be replaced by referencing single files.

[...]
I would keep the type key, since this easily allows future extensions. You can use a map of constructors, e,g.

ctors = { 'file': FileReference.new, 'directory': DirectoryReference.new }
file_ref = ctors[type](data)

A directory reference could be useful if you want to ensure that a directory is completely unmodified, e.g. also no new files.

[stanislaw]

1. This MR has optional `keyword` implemented as was suggested by @sebhub +1

   The keyword could be added to file type references in addition to the lines and stamp support. If keyword is present, then the keyword must be contained in the file content.

I would generalize this to regex.

Please see my change here. Does it work like this?

[stanislaw]

After looking at this piece of code I think we should keep the keyword attribute as is.

I have reverted the keyword regex change by simply removing that commit.

I would keep the type key, since this easily allows future extensions. You can use a map of constructors, e,g.

As of your other change I agree that we should keep the type field to accommodate for the future directory type feature but that should be a separate work because the scope of this MR is quite big already.

Having this said, I suggest that you guys @sebhub @jacebrowning really give it some time of code review and manual testing to see if the MR makes sense because I think we are very close to merging it.

One of the follow-up MRs could be to introduce a deprecation notice for people who might be using the ref feature.

[stanislaw]

This pull request is not getting a review which I guess owes to the fact that the changeset is rather large.

Unfortunately, I cannot split it further because this is what it takes to have multiple references to work with all of the serialization/deserialization, import and exports. One thing I did, however: I have removed all of the GUI work from this PR which should make it a little bit easier to review.

I have preserved a snapshot of the GUI work in a separate branch and since this PR should not affect the behavior of the current 'ref' feature, it should be just fine to first review and (hopefully) merge the logic and then think about the GUI in a separate round.

Could I expect a review from you @jacebrowning ? If you have any concerns about the implementation, code style or any other details, please share. I would like to have some resolution on this issue.

[jacebrowning]

@stanislaw I don't have much bandwidth to review changes, but if this responses to all of @sebhub's feedback them I'm good with moving forward on this.

[sebhub]

Sorry, for the delay. The patch is fine from my point of view. With the type key we have the ability to extend the new references attribute easily on demand.

[stanislaw]

Wow! That's very good news. Thanks @sebhub.

@jacebrowning I assume we are good to merge?

[jacebrowning]

@stanislaw Yeah, feel free to merge approved PRs.