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

Proposal to rename grammarSource option in parse method to source #515

Open
skoji opened this issue Apr 29, 2024 · 2 comments
Open

Proposal to rename grammarSource option in parse method to source #515

skoji opened this issue Apr 29, 2024 · 2 comments
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@skoji
Copy link

skoji commented Apr 29, 2024

I am using the Peggy parser generator library for JavaScript, which has been instrumental in handling complex parsing tasks. However, I have noticed a potentially confusing aspect regarding the parse method.

Currently, the parse method accepts an option named grammarSource. The name suggests that you should use this parameter to specify the name of grammar rules. However, the parameter means the name of the text that the parser is supposed to parse, not the grammar rules.

The name grammarSource could be misleading, so I propose renaming it to source. This change would make it more straightforward that the option is meant for the input text to be parsed rather than the source of the grammar rules.

@hildjj
Copy link
Contributor

hildjj commented Apr 29, 2024

Some issues:

  • Backward compatibility. We would have to accept both source and grammarSource. This makes the next point worse.
  • Namespace pollution. Anyone using source for something else would have their code break, since options for peggy are mixed in with options that the user's grammar needs. If I recall correctly, we thought grammarSource was less likely to collide with existing code.

I more or less agree with you that source would be better, but I think it's too late to make the switch. What we could do is improve the documentation to make it more clear that this is the source that the grammar is operating on, not the source for the grammar.

Let's leave this issue open for suggestions for doc improvements.

@hildjj hildjj added documentation Improvements or additions to documentation good first issue Good for newcomers labels Apr 29, 2024
@hildjj
Copy link
Contributor

hildjj commented Nov 19, 2024

We could also change .format() to look for grammarSource as well as source in the array of objects it gets passed. That would be a relatively easy fix that would get us one benefit: having one variable that contains the source file name.

const grammarSource = 'foo.txt';
const text = 'foo';
try {
  parse(text, {grammarSource});  // Unchanged
} catch (e) {
  console.log(e.format([{grammarSource, text}]); // `grammarSource` instead of `source`
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants