-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fixed #13990/#13991 - reworked platform lookup #7639
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
base: main
Are you sure you want to change the base?
Conversation
if (!filesdir.empty()) { | ||
if (filesdir.back() != '/') | ||
filesdir += '/'; | ||
// TODO: look in filesdir? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we would also look into that folder it could just be one of the lookup paths above.
std::string ppath = Path::fromNativeSeparators(path); | ||
if (ppath.back() != '/') | ||
ppath += '/'; | ||
// TODO: look in platforms first? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the platforms
folder is the proper location for these files it is weird that we do not look there first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed https://trac.cppcheck.net/ticket/13992 about this.
@@ -341,44 +341,40 @@ def test_platform_lookup_builtin(tmpdir): | |||
] | |||
|
|||
|
|||
@pytest.mark.skip # TODO: performs additional lookups when run via symlink in CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem are the CWD lookups because if you run the test from the root it contains the configuration folders and it finds them. If the tests are run from the test folder it will look into that first which does not contain them. So it makes me wonder even more if we should remove those lookups.
The platform loading code is now essentially what will be the shared code for all loading. There is still a few things to resolve and I need to check it against the code in the UI before moving to that. |
# TODO: test with invalid file in project path | ||
# TODO: test with non-file in project path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be addressed in a follow-up to not cramp more stuff into this.
Uh oh!
There was an error while loading. Please reload this page.