-
Notifications
You must be signed in to change notification settings - Fork 30
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
Support loading repositories from files embedded as resources in GirTool #1091
Conversation
afebae3
to
881c473
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my comments are all little nit picks so all in all this is pretty good work, thank you 👍.
In regard to your question: I think it is not a good idea to automatically fall back to the embedded resolver if there is no search path supplied: A current feature of GirTool
is that it allows to create bindings for only a subset of the available platforms.
For example if no search path for windows is supplied the code is generated like always but all the methods are annotated in a way that makes clear that it is not supported under windows. I think it makes sense to keep this feature alive.
The resulting question is how to solve this. One idea would be to have some RepositoryResolverFactory
class which is responsible for delivering some IRepositoryResolver
via a Create
method. So for each platform there can be a repository resolver, but it shouldn't be a problem if there is none, as the code would be annotated, accordingly. (I would like to see a separate class to create IRepositoryResolver
as the GenerateCommand.cs
file is already pretty crammed.)
Perhaps you can come up with some nice idea how to solve this elegantly?
src/Tests/Generation/GirLoader.Tests/ChainedRepositoryResolverTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Generation/GirLoader.Tests/ChainedRepositoryResolverTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Generation/GirLoader.Tests/ChainedRepositoryResolverTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Generation/GirLoader.Tests/ChainedRepositoryResolverTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Generation/GirLoader.Tests/ChainedRepositoryResolverTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Generation/GirLoader.Tests/DisposableTempDirectory.cs
Outdated
Show resolved
Hide resolved
6d9a8a7
to
1fe88c9
Compare
Thanks for the review @badcel, I think I've addressed all of your comments now. I've left the changes as separate commits to help with reviewing, but let me know if you want me to rebase everything down to one commit (and rebase on the new main head). |
Hey, thanks for the update. 🚀 It looks pretty Currently I don't have the time to do a test on my hardware. So the merge has to wait until then (probably in 2-3 days). Can you squash your commits and rebase on the current main branch so everything is ready to be merged as soon as I have the possibility to test on my own hardware? Thank you and sorry for the delay. |
1fe88c9
to
d85a553
Compare
No problem, I've squashed that now thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is good to go!
Resolves #1089
One thing I'm not sure about is whether there should be a more explicit opt in to loading files from the embedded resources. I think the current approach is OK but let me know if you disagree.