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

W-10547474: add constructor to parametrize DialectInstance parsing guessing #478

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

tomsfernandez
Copy link
Contributor

No description provided.

@uip-robot-zz
Copy link

Git2Gus App is installed but the .git2gus/config.json doesn't exist.

@nschejtman
Copy link
Contributor

Why are you parametrizing the Guess? Guesses are Instance specific

Plugins should create Guesses based on wether they can parse an instance or not. You are parametrizing the creation of a Parsing Plugin based on a Guess that should be created by the plugin itself

@tomsfernandez
Copy link
Contributor Author

@nschejtman yes, guesses are instance specific but they rely on the AST to check wether an AST can be parsed by said dialect. I want to be able to parametrize the Guess so that given an AST it ALWAYS applies to my Dialect.

I can't just override the applies method as the guess is also used in Parsing

@tomsfernandez
Copy link
Contributor Author

I guess I could make a plugin from scratch but I'd like to avoid recoding as much of the plugin as possible

@nschejtman
Copy link
Contributor

TBH I don't like this. Guesses are Instance specific, Plugin are not. They should apply to any instance. It's like having a dedicated RAML Plugin for every RAML API.

Since the Guess is what determines the applies method of this plugin, you are fixing the result the the applies on at plugin creation time.

nschejtman
nschejtman previously approved these changes Feb 17, 2022
@tomsfernandez tomsfernandez merged commit f35d3b2 into develop Feb 17, 2022
@tomsfernandez tomsfernandez deleted the W-10547474 branch February 17, 2022 19:31
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