Skip to content

[native_assets_cli] Refactor API #11

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

Merged
merged 9 commits into from
Apr 28, 2023
Merged

[native_assets_cli] Refactor API #11

merged 9 commits into from
Apr 28, 2023

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented Apr 20, 2023

Some open questions:

  • Should we consider Linking and LinkingPreference instead of LinkMode and LinkModePreference?
  • LinkingPreference { static, dynamic, any } means we cannot eagerly fail on the build in the package itself returning an error and non-0 exit code. Maybe that's okay, but it would lead to some wasted work if it's a longer build.
  • hierarchical BuildConfig, discuss in:
  • Programmatic API for BuildOutput Initial version #3 (comment)
    • "It may be better to provide an API that one can use to add things to the output. The API can be hierarchical just like the config." -> What should this API look like? And should we separate API from implementation?

@coveralls
Copy link

coveralls commented Apr 20, 2023

Coverage Status

coverage: 99.841%. remained the same
when pulling 83b937e on address-comments
into 5d5db9e on main.

@dcharkes dcharkes requested a review from mkustermann April 20, 2023 11:05
@dcharkes
Copy link
Collaborator Author

Hey @mkustermann,

I've addressed some of your comments in this PR. PTAL, as well as to the open questions in the PR description.

@dcharkes dcharkes marked this pull request as ready for review April 20, 2023 11:06
@dcharkes dcharkes changed the title Address-comments Refactor API Apr 20, 2023
final String name;
final String description;
final List<Packaging> preferredPackaging;
final List<Packaging> potentialPackaging;
final List<LinkMode> preferredLinkMode;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a List? As we can have only 2 entries and only one of them could be our preferred way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't touch this yet because of :

LinkingPreference { static, dynamic, any } means we cannot eagerly fail on the build in the package itself returning an error and non-0 exit code. Maybe that's okay, but it would lead to some wasted work if it's a longer build.

Let's solve that question first, then I clean this up.

@dcharkes dcharkes changed the title Refactor API [native_assets_cli] Refactor API Apr 28, 2023
@dcharkes dcharkes mentioned this pull request Apr 20, 2023
@dcharkes
Copy link
Collaborator Author

Some of the refactorings need some more discussion, I've opened issues to track these:

Merging this with the first batch of refactorings.

@dcharkes dcharkes merged commit 72a41fe into main Apr 28, 2023
@dcharkes dcharkes deleted the address-comments branch April 28, 2023 13:14
HosseinYousefi pushed a commit that referenced this pull request Nov 16, 2023
HosseinYousefi pushed a commit that referenced this pull request Nov 16, 2023
HosseinYousefi pushed a commit that referenced this pull request Nov 16, 2023
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