Skip to content

Add explicit command line option to limit which files are parsed #4687

Description

@pekkaklarck

When execution just some tests from a big directory structure, parsing only relevant files instead of the whole structure makes parsing faster. We don't currently have any explicit option for limiting parsing like that, but when the --suite option is used files not matching the specified suite are ignored. This functionality won't work too well in the future, though, if we make it possible to change the suite name using the Name setting (#4583). It's also likely that not too many users know about this functionality and the underlying logic to convert file and directory names to suite names for matching purposes is also rather complicated.

A good way to solve all the above problems is adding a separate command line option or options for controlling what files to parse. Once we have such functionality, we can remove the current performance optimization from --suite, a separate option makes the feature easier to discover, and also the code gets simpler. There are some design decisions to be made, though:

  • What should the option be named? I was thinking --includefiles, but it has the same prefix as --include which would mean that shortened usages like --incl that used to be unique wouldn't work anymore. Perhaps --parsefiles or just --files would be better.
  • Should the value be a literal value or should we accept glob patterns? I believe supporting patterns is a good idea.
  • Should the value be just a file name or a relative or absolute path to it? I see benefits with both so possibly we could support both usages.
  • Should the value include file extension? I believe it should be allowed but preferably not required. If that gets too complicated, always requiring it is probably better.
  • Should there be separate option for excluding files? It would make some use cases easier, but I'm not sure is that worth the added complexity.
  • Should this option affect only files or also directories? If the option would match a directory, we could automatically include all files under it, recursively. On the other hand, just matching files would be easier to implement and explain.
  • Should we have separate options for matching files and directories? That's probably too much, especially if we'd have separate options for including and excluding. If directories are considered important, probably better to implement this so that the same option work with both files and directories.

This feature should be implemented before RF 7 where we'd like to change --suite so that it doesn't affect parsing. That would give users who need the performance optimization time to update their scripts.

Activity

  1. added this to the milestone on Mar 14, 2023
  2. changed the title [-]Add explicit command line to limit which files are parsed[/-] [+]Add explicit command line optin to limit which files are parsed[/+] on Mar 14, 2023
  3. changed the title [-]Add explicit command line optin to limit which files are parsed[/-] [+]Add explicit command line option to limit which files are parsed[/+] on Mar 14, 2023
  4. JonathanArns commented on Apr 3, 2023

    @JonathanArns
    Contributor

    Hey, I would like to work this, if possible :)

  5. pekkaklarck commented on Apr 5, 2023

    @pekkaklarck
    MemberAuthor

    Great! Any opinions how this should work?

  6. JonathanArns commented on Apr 5, 2023

    @JonathanArns
    Contributor

    Yea, I like --parsefiles or something similar, since it accurately describes what is happening.
    For functionality, I think either recursive directory or glob support should be there at least, better both.
    I would not worry about being able to omit file extensions at first, and rather try to emulate the way files are specified in other unix commands with glob support.

  7. pekkaklarck commented on Apr 5, 2023

    @pekkaklarck
    MemberAuthor

    Working like other programs accepting patterns like this is a very good design goal. Can you write a bit more detailed spec taking my points in the original description into account? Actually implementing this shouldn't be too hard once we know how it should work.

  8. JonathanArns commented on Apr 6, 2023

    @JonathanArns
    Contributor

    Okay, thinking about it a bit more, I think I had a slightly wrong idea before, because I wasn't considering that it's already possible to specify a list of files and directories for test data. We obviously shouldn't just duplicate that.
    I think then leaning more into just pattern matching file/directory names makes a lot of sense to restrict what parts of a test suite we want to parse.

    So I would propose the following:
    A --parsefiles option that takes a pattern using the same glob-like simple pattern syntax and semantics as what some of the existing options accept (also case-insensitive). It should be possible to specify multiple patterns by using the option multiple times.
    Any file with a filename that matches one of the patterns will be parsed, if it would also be parsed without using the option. In this it should be possible to omit the file extension in the pattern.

    I think also matching directory names with the same option might be nice, but also less intuitive and I would imagine that it's not too much effort in almost all use-cases to just specify the desired directories in the test data argument instead.

    To illustrate the functionality more, let's look at some examples on the following directory structure:

    - tests
    |- one.robot
    |- two.robot
    |- subdir
     |- one.robot
     |- one_two_three.robot
     |- four.robot
    
    robot --parsefiles one .
    - tests
    |- one.robot
    |- subdir
     |- one.robot
    
    robot --parsefiles one* .
    - tests
    |- one.robot
    |- subdir
     |- one.robot
     |- one_two_three.robot
    
    robot --parsefiles two* .
    - tests
    |- two.robot
    
    robot --parsefiles two*.robot .
    - tests
    |- two.robot
    
    robot --parsefiles two*.txt .
    
    robot --parsefiles *two* .
    - tests
    |- two.robot
    |- subdir
     |- one_two_three.robot
    
    robot --parsefiles one --parsefiles two .
    - tests
    |- one.robot
    |- two.robot
    |- subdir
     |- one.robot
    

    If we want a shorthand for --parsefiles, it seems that the obvious -P is taken and giving it -p seems very confusing then. Otherwise I don't have any strong opinions on what the shorthand should be.

    I see you also figured out this contribution will be part of a course 😄

  9. pekkaklarck commented on Apr 6, 2023

    @pekkaklarck
    MemberAuthor

    Sounds good. Some comments below:

    • The option could possibly by named just --files. It would then match the existing --extension.
    • In that case the option certainly should only look at files, not directories, but I believe it would be better also otherwise.
    • Files would make --extension unnecessary because you could use --files *.txt instead or --extension txt. That would allow us later deprecating and removing --extension. I believe it's not too widely used.
    • I feel the value should match the whole name with an extension. If not, --files *.txt wouldn't be same as --extension txt because it would also match foo.txt.robot. Including the extension is also easy and makes the usage explicit.
    • -f would be a natural short option for --files, but I'm not sure is this option used so widely that it needs a shorthand. --extension has -F so there's precedence and -f would also match it.
    • Having a way to not parse certain files could be useful as well. Glob patterns can help there, but that's not too convenient. If we use --files, we could then also support --nofiles. It's probably best to leave it for later, though. It's not clear is it needed and adding it would add complexity.
    • 👍 for courses related to open source and requiring contributions!
  10. JonathanArns commented on Apr 7, 2023

    @JonathanArns
    Contributor

    Thanks for the feedback, all of those sound like very good ideas to me!
    We'll go ahead with an implementation then if that sounds good to you as well :)

    For reference, the plan is a --files or -f option that takes one or multiple (via using the option multiple times) simple glob patterns. When the option is used, the files in the specified data locations are filtered, before parsing, by if their whole filename matches one or more of the patterns.

    We'll not touch the --suite and --extension options for now, but in the future this feature would enable deprecation of --extension and removing the parsing filter functionality from --suite.

  11. JonathanArns commented on Apr 7, 2023

    @JonathanArns
    Contributor

    Actually, one more question remains:
    How should --files and --extension interact with each other?
    I think one has to effectively overrule the other.

    In my mind the best way to do it would be like this:

    • if --files is not used, --extension works as usual
    • if --files is used, then all files matched by --files will be parsed, regardless of the value of --extension

    Otherwise the behavior of --files might seem quite confusing, since a file can only be matched by it, if it is also matched by --extension, even if the option is left blank.

    This also seems sensible if the plan is to eventually deprecate --extension.

  12. removed this from the milestone on May 14, 2023
  13. 11 remaining items

  14. pekkaklarck commented on May 22, 2023

    @pekkaklarck
    MemberAuthor

    PR #4735 adding --files by @JonathanArns and @cetceeve has been merged. Big thanks for contribution!

    We still need to discuss should we support limiting parsing using directories so I'll reopen this issue. Feel free to comment here or on the #devel channel on our Slack.

  15. pekkaklarck commented on May 29, 2023

    @pekkaklarck
    MemberAuthor

    I changed the semantics of --files based on the discussion on Slack and it works as follows nowadays:

    • File names like example.robot and *.robot are matched against parsed file names as earlier.
    • Files can also be specified using using absolute and relative paths. Paths support glob patterns, including recursive matching using the ** syntax.
    • If --files is used with a directory path, all files inside the specified directories are included, recursively.

    I believe these semantics ought to cover all the current needs, but in the future there may be desire to allow also excluding files.

    The remaining design decision is the name of the option. --files is a bit add when you can also use the option with directories. Something like --parse-include or --parse-only could be better. I like the latter name more on its own, but if we want to support excluding files then --parse-include and --parse-exclude would be a good pair.

  16. d-biehl commented on May 29, 2023

    @d-biehl
    Contributor

    Looks good at first sight, but I need a few days to test it and integrate it into RobotCode.
    I would prefer --parse-include as the name of the option, then it's easier to introduce the --parse-exclude option later and it matches the 'normal' --include and --exclude options.

  17. pekkaklarck commented on May 29, 2023

    @pekkaklarck
    MemberAuthor

    The name has been changed to --parse-include. I still need to update the User Guide and think a bit more how this should work with --extension.

    It would be great if you @d-biehl could test the new semantics soon!

  18. pekkaklarck commented on May 29, 2023

    @pekkaklarck
    MemberAuthor

    Few more enhancements still:

    • Files having extensions used with --parse-include are parsed even though they wouldn't normally be. This means that --extension *.rst is enough to parse reST files and additional --extension rst isn't needed. This makes --extension redundant and it can be deprecated in the future.
    • --parse-include got -I as a short option.
    • User Guide documentation has been updated.

    After these changes I consider this issue finally done for good. It can still be reopened if testing by @d-biehl or others reveals problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions