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

Move foreign code from global scope to dub.internal #2525

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

s-ludwig
Copy link
Member

@s-ludwig s-ludwig commented Nov 2, 2022

Avoids conflicting with the actual dub packages when working with dub as a library. Also, even if just by convention, it ensures that dependent code doesn't rely on this code as a public API.

Avoids conflicting with the actual dub packages when working with dub as a library. Also, even if just by convention, it ensures that dependent code doesn't rely on this code as a public API.
@s-ludwig s-ludwig force-pushed the make_foreign_code_internal branch from d273175 to 8bd3aee Compare November 2, 2022 11:03
@s-ludwig
Copy link
Member Author

s-ludwig commented Nov 2, 2022

BTW, I think the cost/benefit ratio of the whole dyaml/Configy based refactoring is highly debatable. Even the complexity of the dub code itself doesn't really seem to have become lower, but these libraries add a substantial amount of code for maybe a few lines less, if any.

@WebFreak001
Copy link
Member

I think nice advantages of dyaml/configy over the manual parsing code is that

  • we don't have to maintain custom deserialization logic anymore
  • we get much better automatic parse and deserialization errors

Personally I see a future in mir / mir-ion as a generic (de)serialization framework, so I would like to push that much more for general purpose use, but we have configy working here in DUB + it gives us much nicer errors right now. Also having YAML as format allows us to extend the settings format, which has long been an open issue.

@WebFreak001 WebFreak001 merged commit fd04494 into master Nov 2, 2022
@WebFreak001 WebFreak001 deleted the make_foreign_code_internal branch November 2, 2022 18:19
@Geod24
Copy link
Member

Geod24 commented Nov 3, 2022

@s-ludwig : The main motivation for using configy / dyaml was scalability. While the upfront cost was high (introducing the libraries themselves), switching individual files has been almost painless. Adding support for YAML is trivial.

On the code quality side, introducing a library gives us the power of abstraction. It allowed me to isolate places where abstraction was breached (e.g. #2277 ) and fix them. This could have also been done by building a library inside of dub, but what I would have ended up with would have been, in spirit, quite similar to Configy. So using it just saves us a bunch of time.

Finally, on the code size front, the reason why we only saved "a few lines" is that I kept the JSON parsing code in for now, as a fallback in case there is a bug with Configy (so far I haven't found one). When the transition period ends, then we will remove large portions of code.

On a more practical note, what @WebFreak001 said.

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.

3 participants