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

Make BaseNode init accessible outside the Down module #197

Closed
wants to merge 1 commit into from
Closed

Make BaseNode init accessible outside the Down module #197

wants to merge 1 commit into from

Conversation

daikini
Copy link

@daikini daikini commented Jan 3, 2020

This pull request adds the public designator to the BaseNode.init initializer so that it can be called outside the Down module.

Fixes #196

@codecov
Copy link

codecov bot commented Jan 3, 2020

Codecov Report

Merging #197 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #197   +/-   ##
=======================================
  Coverage   32.59%   32.59%           
=======================================
  Files          74       74           
  Lines        2362     2362           
=======================================
  Hits          770      770           
  Misses       1592     1592
Impacted Files Coverage Δ
Source/AST/Nodes/BaseNode.swift 92.3% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd1a3f6...d4f6b45. Read the comment docs.

@iwasrobbed iwasrobbed requested a review from johnxnguyen January 3, 2020 20:22
@johnxnguyen
Copy link
Owner

Hi @daikini , thanks for the pull request. Could you elaborate on what problem this change solves?

@daikini
Copy link
Author

daikini commented Jan 4, 2020

Hi @johnxnguyen, I'm writing a new DownRenderable compatible renderer that uses theDocument class to traverse the AST tree with my custom Visitor. It is not currently possible to instantiate a Document outside of the Down framework because by default Swift treats the init method private outside the Down framework even though the Document class is itself public. The init method that Document uses is in its parent class the BaseNode class.

This PR fixes this issue by making the BaseNode.init method explicitly public so that it (and its subclasses) can be instantiated outside of Down.

Without this PR it is not possible to build new renderers for Down outside Down itself.

@iwasrobbed
Copy link
Collaborator

@daikini That's fair; as API design goes, it's always good to be careful about opening internal APIs up for use, but at the same time we should encourage custom renderers so the community can build their own use-case-specific tooling

Thanks for the extra detail 👍

@iwasrobbed iwasrobbed self-requested a review January 4, 2020 17:43
Copy link
Collaborator

@iwasrobbed iwasrobbed left a comment

Choose a reason for hiding this comment

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

As long as @johnxnguyen approves (versus an alternative approach), this seems reasonable to me

@johnxnguyen
Copy link
Owner

Glad to hear you’re making a custom renderer! I’m a bit hesitant to open the access of the initializer, because the node classes were never intended to be invoked outside of the framework. The reason being is because the CMarkNode is just a pointer to a C struct, and initializing a particular node class with the wrong pointer could lead to unexpected behavior.

To create the correct node class from a given CMarkNode instance, there is a handy extension method on CMarkNode called wrap() see here which will instantiate and return the node class (albeit type erased to the Node protocol type).

You could for instance do something like:

let down = Down(markdownString:...)
let document = try? down.toAST().wrap() as? Document
...

If this approach is too inconvenient, I would suggest to create a helper method or add a renderer that returns the Document node (just like the AST renderer returns the CMarkNode), instead of opening access for every BaseNode subclass.

@daikini
Copy link
Author

daikini commented Jan 4, 2020

@johnxnguyen, didn't know about the wrap() method. That works great. 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.

'BaseNode' initializer is inaccessible due to 'internal' protection level
3 participants