Skip to content

Combine const/enum with type error messages#169

Merged
jdesrosiers merged 6 commits intohyperjump-io:mainfrom
Sam-61s:feat/combine-type-const/enum-message
Feb 25, 2026
Merged

Combine const/enum with type error messages#169
jdesrosiers merged 6 commits intohyperjump-io:mainfrom
Sam-61s:feat/combine-type-const/enum-message

Conversation

@Sam-61s
Copy link
Contributor

@Sam-61s Sam-61s commented Feb 18, 2026

Fixes #164

When validation fails with both type and const/enum keywords, this PR shows only the more specific const/enum error with type-aware value filtering.

Changes:

  • Enhanced constEnum handler to filter allowed values by type constraints
  • Modified type handler to suppress errors when const/enum is present

Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

It didn't occur to be that you'd have to duplicate the logic for determining the type in the constEnum handler. That tells me that my original thought that this should be one combined typeConstEnum handler is probably better.

@Sam-61s
Copy link
Contributor Author

Sam-61s commented Feb 20, 2026

It didn't occur to be that you'd have to duplicate the logic for determining the type in the constEnum handler. That tells me that my original thought that this should be one combined typeConstEnum handler is probably better.

Now, I've combined the handlers into a single typeConstEnum handler with a shared resolveTypes() helper function to eliminate the duplicated type-resolution logic.

@Sam-61s Sam-61s force-pushed the feat/combine-type-const/enum-message branch from ddb5384 to 330969b Compare February 20, 2026 12:47
Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

This looks way more complicated than it should be. Can you see if it can be simplified? Also, I'd like to see a few more tests. This is a complicated feature especially when it comes to schema locations. Let's see if we can cover some more edge cases.

Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

The added tests look good. I left a suggestion to try to help simplify the filtering approach.

Comment on lines +60 to +63
const keyword = await getSchema(schemaLocation);
const keywordJson = new Set([jsonStringify(/** @type Json */ (Schema.value(keyword)))]);

allowedJson = allowedJson?.intersection(keywordJson) ?? keywordJson;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do the filtering as we go, we can skip that whole large block of code dedicated to filtering. I think this code is more clear and concise than filtering after.

Suggested change
const keyword = await getSchema(schemaLocation);
const keywordJson = new Set([jsonStringify(/** @type Json */ (Schema.value(keyword)))]);
allowedJson = allowedJson?.intersection(keywordJson) ?? keywordJson;
const keyword = await getSchema(schemaLocation);
const keywordJson = new Set();
if (allowedTypes.has(Schema.typeOf(keyword))) {
keywordJson.add(jsonStringify(Schema.value(keyword)));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored the const/enum collection to filter by type inline, as suggested.

@Sam-61s Sam-61s requested a review from jdesrosiers February 24, 2026 09:23
Copy link
Collaborator

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

Yeah, that looks much better. I left a few more note for things that could be improved.

@Sam-61s
Copy link
Contributor Author

Sam-61s commented Feb 25, 2026

cleanup improvement done..

  1. removed duplicate integer cleanup
  2. used Schema.iter & Schema.typeOf for enum
  3. removed unnecessary early return
  4. consolidate all returns at the end

@jdesrosiers jdesrosiers merged commit d610291 into hyperjump-io:main Feb 25, 2026
1 check passed
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.

Combine const/enum with type error handling

2 participants