Skip to content

Conversation

wowi42
Copy link
Contributor

@wowi42 wowi42 commented Sep 25, 2025

Fix #1454

  • Pull request is based on the default branch (3.x at this time)
  • Pull request includes tests for any new/updated operations/facts
  • Pull request includes documentation for any new/updated operations/facts
  • Tests pass (see scripts/dev-test.sh)
  • Type checking & code style passes (see scripts/dev-lint.sh)

@NichtJens
Copy link
Contributor

NichtJens commented Sep 25, 2025

Not sure if the discussion is better in #1454 or here.

I am not sure this is entirely correct now. Take the case where the file does exist but is empty. For instance something like: touch somefile. This is also a None now. Should it be?


Also, I think this would be better:

    def process(self, output):
        if not output:
            return self.default()
        return output

... if I understood the logic of FactBase correctly.

@wowi42
Copy link
Contributor Author

wowi42 commented Sep 26, 2025

@NichtJens You are right, fixed it

@NichtJens
Copy link
Contributor

@wowi42 Does this already take my other question into account?

Take the case where the file does exist but is empty. For instance something like: touch somefile. This is also a None now. Should it be?

@wowi42
Copy link
Contributor Author

wowi42 commented Sep 26, 2025

Nope, I was reading too quickly. I'm unsure how to differentiate between no file and an empty file. Perhaps we should involve @Fizzadar for his input.

@NichtJens
Copy link
Contributor

Yes, this is a problem. Actually, it's exactly why I didn't propose a PR similar to yours and just opened an issue instead. I should probably have specfied what I had tested already. Sorry about that.


Btw., from my experiments, you can go back to the old:

def process(self, output):
    return output

... and it works the same as your current code. The magic happens in get_fact, I think.


In fact, the reason I am asking is since I have the same problem with what I am currently working on. This was also the reason I posted the issue in the first place. I need to update the contents of a json file on the remote side. For this, I wanted a fact that gets the json contents and came up with this:

class JSONFileContents(files.FileContents):
    """
    Returns the contents of a JSON file as dict.
    Returns an empty dict if the file does not exist or cannot be parsed as JSON.
    """

    default = dict

    def command(self, path):
        return make_formatted_string_command("cat {0} || true", QuoteString(path))

    def process(self, output):
        output = super().process(output)
        output = "\n".join(output)
        try:
            output = json.loads(output)
        except json.decoder.JSONDecodeError:
            return self.default()
        return output

This is very similar to yours, except I just kept the cat error message in. I don't think that hurts.


I think these two changes make the PR very minimal (single line change, plus tests). But I am still not sure what the desired behavior really is.

@Fizzadar
Copy link
Member

Fizzadar commented Oct 1, 2025

The expected behaviour is:

  • if no file, return None
  • if empty file, return ""

The FindInFile fact solved this same problem already: https://github.com/pyinfra-dev/pyinfra/blob/3.x/src/pyinfra/facts/files.py#L431. Basically we echo a specific string if the file does not exist which we can then use to return the None vs empty string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileContents does not return None if file does not exist
3 participants