Skip to content

Consider restoring the old Eval plugin #1276

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

Closed
pepeiborra opened this issue Jan 30, 2021 · 7 comments
Closed

Consider restoring the old Eval plugin #1276

pepeiborra opened this issue Jan 30, 2021 · 7 comments
Labels
component: hls-eval-plugin status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet

Comments

@pepeiborra
Copy link
Collaborator

The extended Eval plugin was merged in #438 replacing the original Eval plugin. It is capable and powerful, but also complicated and dangerous - its custom parser has a tendency to error out and bring down the entire process, and the additional features can be hard to understand.

Many users are not aware of the extra features for running doc tests and properties, changing settings and imports, etc. and would be better served by the simpler original Eval plugin, but unfortunately it's now gone.

Proposal: bring back the original Eval plugin under a new name, e.g. hls-eval-simple, and ship with both linked and config to select which one gets used, just like we do with formatters.

@konn
Copy link
Collaborator

konn commented Jan 30, 2021

👍 for it. Although its custom parser is rewritten carefully in #1232 (not merged though), maintaining its own parser and try not to introduce infinite loops to freeze entire HLS at the same time can be a complicated process.
Aside from extended features like block comment support, QuickCheck support and $setup section and so on, there can be many common parts that can be shared both simple- and extended-eval plugin. So I think it makes sense to me to separate extended feature as a separate package and make it depend on hls-eval-simple.

Hopefully, changes in #1232 will stabilise comment parsing of Extended Eval Plugin, I think it is safe to set the default Eval plugin to Extended mode provided that the PR gets merged.

@jneira
Copy link
Member

jneira commented Jan 30, 2021

Ok, #1232 sounds promising, i guess we should make a minor release once it is merged

@jneira jneira added component: hls-eval-plugin status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: refactor labels Jan 30, 2021
@Ailrun
Copy link
Member

Ailrun commented Jan 31, 2021

@jneira I think since we already merged #704, releasing a minor version in a few days goes somewhat against the reason why we wait the merge of the PR.

@jneira
Copy link
Member

jneira commented Jan 31, 2021

yeah, we should release a version with the eval fix but without hiedb, maybe it does not worth

@konn
Copy link
Collaborator

konn commented Feb 1, 2021

I want to confirm: not within a few days, but February release willbe made once hiedb change works properly, right?

@Ailrun
Copy link
Member

Ailrun commented Feb 1, 2021

@konn Yeah, I believe that's the original intention.

@michaelpj
Copy link
Collaborator

Closing since nothing has happened and it seems unlikely we're going to do this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: hls-eval-plugin status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet
Projects
None yet
Development

No branches or pull requests

6 participants