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

Improve error messages when workflow settings have non-string values for default project or dataset #1846

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kolina
Copy link
Contributor

@kolina kolina commented Sep 14, 2024

Currently if defautProject or defaultDataset have an integer type in workflow_settings.yaml, a compilation will throw a not very obvious error like t.target.schema.includes is not a function. I've added a validation of these fields to be strings (if not, an explicit error is thrown).

@kolina kolina requested a review from Ekrekr September 14, 2024 23:55
Copy link
Contributor

@Ekrekr Ekrekr left a comment

Choose a reason for hiding this comment

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

Currently if defautProject or defaultDataset have an integer type in workflow_settings.yaml, a compilation will throw a not very obvious error like t.target.schema.includes is not a function. I've added a validation of these fields to be strings (if not, an explicit error is thrown).

I think there should be a better fix for this:

I'm not sure why we're not catching it already - but fixing the proto validation would be the most extensible solution!

@kolina
Copy link
Contributor Author

kolina commented Sep 16, 2024

I also hoped that it should have been the case, but unfortunately this code

const proto = protoType.create(object);
const protoCastObject = protoType.toObject(proto);
doesn't validate types and actually allows to create protoCastObject where defaultDataset has an integer type and this check later doesn't work
if (typeof desiredValue !== typeof presentValue) {

The thing that actually verifies types is protoType.verify(). WDYT about using it?

@Ekrekr
Copy link
Contributor

Ekrekr commented Nov 11, 2024

.verify() is a lie, it doesn't actually do anything - there are no situations where the call will actually fail.

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.

2 participants