lib50.files returns all files when arg is empty list

[dmalan]

Below seems to return all files of the form /some/path/* rather than nothing?

print(lib50.files([], root="/some/path"))

[dmalan]

Okay, on further testing, a .cs50.yml file like

lab50: true

or even

lab50:
  files:
    - !include hello.c

also results in lib50.files returning unexpected paths. E.g., if I have:

$ ls -a
.cs50.yml  hello.c  README.md

the above YAML results in lib50.files returning both hello.c and README.md. Only if I change the YAML to

lab50:
  files:
    - !include hello.c
    - !exclude README.md

is README.md successfully excluded?

[Jelleas]

The default is include everything (similarly to .gitignore) as per:

lib50/lib50/_api.py

Line 134 in 3005b61

# Include everything by default

You should be able to exclude just README.md with:

lab50:
  files:
    - !exclude README.md

[cmlsharp]

Yep, what Jelle said. This ensures that our promise that a minimal check50: true actually defines a valid check.

[dmalan]

I see, I presume this is what you were alluding to when you proposed I blacklist README.md. Alternatively, perhaps I should prepend an implicit

- !exclude *

for lab50, so that all files are excluded by default? Else, for labs with not just README.md but also checks, you'll end up having to !exclude more files than you !include potentially? Though this would invert the behavior of files in lab50 vis-a-vis check50/submit50. What do you think?

[Jelleas]

That would make the default case meaningless though, which I guess is:

lab50: true

As all files are then excluded.

Rather, and for this I should probably pick up on #8, I'd expect the default behavior to be: include everything via !open (or !opened), !exclude specifically README.md. Such that all files do end up in the lab50 environment, and it's very clear what's there.

In the case of excluding more than including, anyone can just invert the logic by prepeding !exclude "*" to files:.

[dmalan]

Hm, but many/most labs will likely end up having checks. So I'm not sure we're optimizing for the common case here? With submit50, it's reasonable to submit everything in a student's dir by default. But with labs, including everything is likely a mistake, since you wouldn't want

• README.md
• any .png, .gif, .jpg images used in README.md
• __init__.py or any other check-related files

copied into the sandbox by default. Indeed, odds are you just want one file (e.g., foo.c) copied in. So it does feel like this logic should be inverted for labs.

Can/should we add a flag to lib50.files that allows caller to specify whether * should be included or excluded by default? Else most every lab is now going to need !exclude "*".

[Jelleas]

lib50.files currently takes in a list of patterns, the caller can then add patterns to this list. So for lab50 you would prepend the pattern for !exclude "*".

However, the interface is pretty bad right now, let me see if I can find some time tomorrow to work on this. I envision you would end up doing something like this:

# Read .cs50.yaml
with open(".cs50.yaml") as f:
    content = f.read()

# Configure a config loader
loader = lib50.Loader("lab50")
loader.scope("files", "closed", "opened", default="exclude")  

# Load
parsed_yaml = loader.load(content)

# Manipulate the patterns specified in .cs50.yaml
file_patterns = [lib50.TaggedValue("exclude", "*")] + parsed_yaml["files"]

# Tell lib50.files how to treat the custom tags
included, excluded = lib50.files(file_patterns, include_tags = ["opened", "closed"], exclude_tags = ["exclude"]) 

ps. Should it be open, close and exclude, or opened, closed and excluded?

[Jelleas]

Working prototype with:

pip install git+https://github.com/cs50/lib50.git@develop

import lib50

# Init loader
loader = lib50.config.Loader("lab50")

# Allow only values under files to be tagged with open, close, exclude
loader.scope("files", "open", "close", "exclude")

# Load
with open(".cs50.yaml") as f:
    config = loader.load(f.read())

# Prepend !exclude "*"
config["files"] = [lib50.config.TaggedValue("*", "!exclude")] + config["files"]

# Tell lib50.files how to treat the custom tags
included, excluded = lib50.files(config["files"],
                                 include_tags=[],
                                 exclude_tags=["exclude"],
                                 require_tags=["open", "close"])

You can opt to use include_tags over require_tags for !close and !open. The difference is that require_tags does not take patterns, only filepaths. And it requires those files to be present, otherwise a MissingFilesError is raised.