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

🎨 Improving API structure for readability #460

Merged
merged 2 commits into from
Aug 23, 2018
Merged

🎨 Improving API structure for readability #460

merged 2 commits into from
Aug 23, 2018

Conversation

cHaLkdusT
Copy link
Contributor

Updating API signature when using SVGParser to avoid confusion. Should be resource instead of path

@ystrot ystrot self-assigned this Aug 17, 2018
@ystrot
Copy link
Member

ystrot commented Aug 17, 2018

Hey Julius!

Thank you for your work. We already had similar discussion regarding this method: #269 (comment)

So let's me briefly go through my points:

  • Macaw doesn't break API in minor versions. So the first step would be to deprecate existing parse methods.
  • Let's add the new method compatible with Bundle.path/Bundle.url:
open class func parse(resource: String, ofType: String = "svg", inDirectory: String? = nil, fromBundle: Bundle = Bundle.main) throws -> Node

Note that Swift using forResource name, because it's correct to say "path for resource", but will be wrong to say "parse for resource". So in our case it should be resource. Also it would be better to make fromBundle the last attribute to use main by default.

  • Finally we need another method:
open class func parse(fullPath: String) throws -> Node

which will be used in other methods.

I would be happy to apply your PR if you'll update it as I suggested. Also feel free to comment if you don't agree 😊

* Updated SVGParserError to conform to Equatable so we can throwing functions
@cHaLkdusT
Copy link
Contributor Author

Hi Yuri,

I have now made the changes as per your comment.

✨ Implemented open class func parse(resource: String, ofType: String = "svg", inDirectory: String? = nil, fromBundle: Bundle = Bundle.main) throws -> Node
✨ Implemented open class func parse(fullPath: String) throws -> Node
✅ Added unit test for the new API

Cheers,
Julius

@ystrot ystrot added this to the 0.9.3 milestone Aug 23, 2018
@ystrot ystrot merged commit ba6512e into exyte:master Aug 23, 2018
@ystrot
Copy link
Member

ystrot commented Aug 23, 2018

Thank you!

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.

2 participants