Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rwlock: require info keys to be present in schema #3283

Merged
merged 1 commit into from Feb 5, 2020
Merged

rwlock: require info keys to be present in schema #3283

merged 1 commit into from Feb 5, 2020

Conversation

ghost
Copy link

@ghost ghost commented Feb 5, 2020

Previous implementation allowed incomplete info:

SCHEMA({
    "write": {
        "file_1": {"pid": 1, "cmd": "exec"},
        "file_2": {"pid": 2},
        "file_3": {"cmd": "..."},
        "file_4": {},
    },
    "read": {
        "file_1": [{"pid": 1}],
    }
})

@ghost ghost requested a review from efiop February 5, 2020 00:32
dvc/rwlock.py Outdated Show resolved Hide resolved
dvc/rwlock.py Outdated Show resolved Hide resolved
@ghost ghost requested a review from efiop February 5, 2020 16:49
dvc/rwlock.py Outdated
Comment on lines 17 to 18
Required("write", default={}): {str: INFO_SCHEMA},
Required("read", default={}): {str: [INFO_SCHEMA]},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these used to be Optional(default) but now they are Required. Is this intended?

Sure, de-facto we are always setting even empty dicts for these down below, so it won't break anything in dvc itself. But on the other hand, these are not required by our reader and it is not required in terms of format, so I don't really see a point in making them Required now. The problem you've described in the PR description is fully fixed by INFO_SCHEMA adjustment you've done above. If you want to get rid of defaultdict then let's Optional(..., default={}) here instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efiop , I think Required and Optional are the same when a default is provided, is just semantically different.

I'll make the change, since it makes more sense in that regard.

@ghost ghost changed the title rwlock: apply a stricter schema rwlock: require info keys to be present in schema Feb 5, 2020
Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@efiop efiop merged commit 509ce3e into iterative:master Feb 5, 2020
@ghost ghost deleted the rwlock-schema branch February 5, 2020 18:05
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.

1 participant