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

Fix path and naming issues with intertype codegen #95

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

fivegrant
Copy link
Collaborator

@fivegrant fivegrant commented Dec 18, 2023

Addresses a few things I encountered in an MR PR:

  • JSON schema generation ignores path argument
  • Module name is basename of the file (strips parent directory)

BEFORE MERGING, we would ideally merge this very small refactor into this PR. I separated it out just in case you didn't actually want it. now merged

Copy link
Member

@jpfairbanks jpfairbanks left a comment

Choose a reason for hiding this comment

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

Just waiting for @olynch to see my question about unforeseen consequences of taking that basename. I think the fix is correct, but wanted to double check. I am happy to also rename the methods and switch to using multiple dispatch. That will be a breaking API change but it is better to break now, before we get users.

src/intertypes/json.jl Show resolved Hide resolved
@@ -307,7 +307,7 @@ end

function include_intertypes(into::Module, file::String, imports::AbstractVector)
endswith(file, ".it") || error("expected a file ending in \".it\"")
name = Symbol(chop(file; tail=3))
name = Symbol(basename(chop(file; tail=3)))
Copy link
Member

Choose a reason for hiding this comment

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

@olynch, any unforeseen consequence of this with respect to hierarchically named intertypes modules? We don't actually want path separators in these names right?

* Dispatch instead of separate functions for module create

* Dispatch on singleton subtypes
Copy link
Collaborator

@olynch olynch left a comment

Choose a reason for hiding this comment

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

This all looks good, thanks @fivegrant!

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (813a4e1) 90.57% compared to head (e8d4db4) 90.33%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
- Coverage   90.57%   90.33%   -0.25%     
==========================================
  Files          22       22              
  Lines        2059     2059              
==========================================
- Hits         1865     1860       -5     
- Misses        194      199       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@olynch olynch merged commit 751a01c into AlgebraicJulia:main Dec 19, 2023
9 checks 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.

None yet

3 participants