Skip to content

feat: allow dash as ID_STRICT for imports #57

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

Conversation

MiriamGaus
Copy link
Contributor

fixes #56

@SundermannC
Copy link
Member

Thanks a lot for your PR and all your contributions to UVL over the last months in general @MiriamGaus!
This is very much appreciated.
I think this particular change may cause ambiguities. We suppressed all allowed arithmetic operators in non-enclosed feature names to prevent the ambiguity between having a two features connected by an operator (without spaces) and a single reference. I have not tested it yet but the expression in the constraint below should be parsed to the non-existing feature with the name "A-B", which may not be intended. Maybe it would be fine to enforce spaces between the feature names. Maybe we could also enable the non-strict ids with "import-name" in imports? What do you think?

features
     Root
       optional
           Integer A
           Integer B
constraints
     A-B == 0

@MiriamGaus
Copy link
Contributor Author

MiriamGaus commented Jun 6, 2025

You're right. The given model isn't parsed, although it should be possible to parse. With enforcing spaces there is no ambiguity anymore. I'm not sure whether this is a wanted solution. Even though spaces provide a better readability for the constraints.
I think non-strict ids don't fix the problem because if there is no alias enforced the dash remains in the identifier and causes the same ambiguity problems.

@SundermannC
Copy link
Member

I am not sure about enforcing spaces as well. I do not hate it in general, but it would probably require more substantial changes in the way we parse.
Regarding non-strict ids, also enforcing " ", just as we do for features, should work right? Or is there an issue I am missing?
For example, allowing the following should resolve the issue, right?

imports
    "name-with-dash"

features
   "name-with-dash".Root

@MiriamGaus
Copy link
Contributor Author

MiriamGaus commented Jun 6, 2025

I think this could fix the issue, I tried it and this already works. This should be added to the documentation.
Another solution could be to enforce an alias without a dash in it. This should fix the problem as well.
Is it okay to change the semantics of the UVL parser? For example to your given solution.

@MiriamGaus MiriamGaus force-pushed the refactor-names-and-string-attributes branch 2 times, most recently from 6d769ec to 9863d8e Compare June 8, 2025 09:02
@MiriamGaus MiriamGaus force-pushed the refactor-names-and-string-attributes branch from 9863d8e to 34a3658 Compare June 8, 2025 09:03
@SundermannC
Copy link
Member

SundermannC commented Jun 17, 2025

I am heavily in favor of applying your proposed change, but I also think we should be generally cautious when changing the semantics of the language/ the base parser. We should probably get feedback of more involved parties on this prior to merging.

Edit: I just understood that this required no change to the grammar at all. Then, I think we can merge right away.

Copy link
Member

Choose a reason for hiding this comment

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

Even though I do not mind this change here, we probably do not want to adapt files for visual structure in unrelated commits in general.

Copy link
Member

@SundermannC SundermannC left a comment

Choose a reason for hiding this comment

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

Thanks for updating the documentation. Looks good overall, I just have small change requests.

@MiriamGaus
Copy link
Contributor Author

With the new bigger changes for the uvl parser, this PR becomes irrelevant and can be closed

@MiriamGaus MiriamGaus closed this Jul 21, 2025
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.

dash isn't allowed in the name of a .uvl-file
2 participants