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

Cannot pass custom InputPos #6

Closed
ferdinand-beyer opened this issue May 24, 2023 · 7 comments
Closed

Cannot pass custom InputPos #6

ferdinand-beyer opened this issue May 24, 2023 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@ferdinand-beyer
Copy link

I've tried to create a custom InputPos and pass it via the :pos option to p/parse. Unfortunately, the resulting positions are always nil.

I've tracked it down to the :default method of init-pos:

(defmethod init-pos :default
  [{:keys [pos]} _]
  (when (keyword? pos)
    (throw (ex-info (str "Cannot init input position for: " pos) {}))
    pos))

The when should probably be an if, to use the passed pos if it is not a keyword.

Side question: Is there a recommended way to construct a TextPos, since strojure.parsesso.impl.pos is a private namespace? Alternatively, could we pass :line and :col options to initialize the TextPos with values other than 1? This is useful in a multi-pass scenario, when we want to pass a known position to a sub-parser (e.g. parsing a XML attribute value, where we already know the attribute node's position).

@serioga serioga self-assigned this May 25, 2023
@serioga
Copy link
Member

serioga commented May 25, 2023

I've tried to create a custom InputPos and pass it via the :pos option to p/parse. Unfortunately, the resulting positions are always nil.

Can you provide an illustrating code fragment?

@serioga
Copy link
Member

serioga commented May 25, 2023

could we pass :line and :col options to initialize the TextPos with values other than 1?

yes, I will add initialization of values from options of :text pos

@serioga
Copy link
Member

serioga commented May 25, 2023

Looks like current implementation works only with keyword pos extendable via adding pos/init-pos methods.
So docstring is misleading about this.
I think its OK to have only one way to extend without direct usage of InputPos on user side.
Let's try to write code you need, and see.

@ferdinand-beyer
Copy link
Author

Can you provide an illustrating code fragment?

Sure, here is an example:

(require '[strojure.parsesso.parser :as p]
         '[strojure.parsesso.impl.pos :as pos])

(p/parse (p/get-state :pos) "" {:pos :text}) ; => {:tab 8, :line 1, :col 1}
(p/parse (p/get-state :pos) "" {:pos (pos/->TextPos 8 7 13)}) ; => nil

So I wanted to tell the parser not to start at 1,1, but 7,13. As you can see, the position is reported as nil.

The init-pos sees the TextPos, checks for an unsupported keyword, and returns nil because of the when. I'm pretty sure if we change the when to if, this would work, as init-pos would return the TextPos unchanged.

Ideally, I would not need to construct the TextPos myself, as this requires using the impl namespace. Instead, this would work for me:

(p/parse (p/get-state :pos) "" {:pos :text, :line 7, :col 13}) ; => {:tab 8, :line 7, :col 13}

Should be quite easy, as the init-pos method for :text can look for the :line and :col options.

Let me know if it would help if I wrote a PR!

@ferdinand-beyer
Copy link
Author

I currently work around it by providing a custom method:

(defmethod pos/init-pos ::text
  [{:keys [tab line col] :or {tab 8, line 1, col 1}} _]
  (pos/TextPos. tab line col))

(p/parse (p/get-state :pos) "" {:pos ::text, :line 7, :col 13}) ; => {:tab 8, :line 7, :col 13}

serioga added a commit that referenced this issue May 25, 2023
@serioga serioga added the bug Something isn't working label May 25, 2023
@serioga serioga closed this as completed May 25, 2023
@serioga
Copy link
Member

serioga commented May 25, 2023

@ferdinand-beyer thank you for report 🙇

@ferdinand-beyer
Copy link
Author

Thanks for improving this!

Small nitpick for a rainy day: You forgot to update the :arglists -- they contain tab but not line and col :)

  {:arglists '([p input]
               [p input {:keys [pos, tab, user-state] :as options}])}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants