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

Fix #12758: Add markup laguages #6421

Closed
wants to merge 1 commit into from
Closed

Conversation

olabetskyi
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator

This maybe requires all negative language checks to be replaced with positive ones.

@olabetskyi
Copy link
Collaborator Author

This maybe requires all negative language checks to be replaced with positive ones.

Yeah I've thought about it

@danmar
Copy link
Owner

danmar commented May 22, 2024

@firewave a customer had problems when scanning a qml project. There was an assertion error when Language was None.

I think some kind of test should be added when the qml file is not handled properly.

@olabetskyi
Copy link
Collaborator Author

@firewave a customer had problems when scanning a qml project. There was an assertion error when Language was None.

I think some kind of test should be added when the qml file is not handled properly.

It's actually the oposite. It wasn't None. It was set as C++ by default and when we call setLang function to change language for markup file it fails.

@firewave
Copy link
Collaborator

I think some kind of test should be added when the qml file is not handled properly.

Yes, a test case would be good to see what is actually going on. This feels like another bandaid fix.

Also QML needs more test coverage.

I promised to provide a PR to disable the asserts for the release but I forgot. But somehow it is good that we expose these issues. Ignoring them is not good and just makes things worse down the line.

@firewave
Copy link
Collaborator

This maybe requires all negative language checks to be replaced with positive ones.

I still had a branch with such changes but while reviewing it I no longer was sure if that is a good change. It would dependent on the code. So we should not touch that unless we need to.

Making it a tri-state would make things much more complicated.

Currently we are treating markup as C so that means all code which is isC() == true would be executed. So introducing an additional language would cause none of that code to be executed which will most likely lead to unexpected results.

I think the markup stuff needs to be handled differently and much easier.

@firewave
Copy link
Collaborator

Ah - I see the actual problem.

The user specified --language which causes the Settings::enforcedLanguage to be applied in the constructor. Thus the subsequent TokenList::setLang() call fails.

I did consider this and wanted to add the language to the constructor but something was preventing that at the time.

I will look into providing a much less intrusive fix.

@firewave
Copy link
Collaborator

See #6425.

@firewave
Copy link
Collaborator

Closing in favor of #6425.

@firewave firewave closed this May 23, 2024
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