Improve file loading transparency #403
Conversation
|
Thanks for proposed changes! I think the code, as such, looks really good. But I have two comments: 1. Bump c++ version So this part either has to be changed to comply with c++ 14, or we would need to hold this PR until we know the bump to c++ 17 is OK. 2. Missing case However, to my understanding (and I might be wrong), there is another case being lost: Search for catalog directly in explicitly specified path (via --path argument). Which has both pros and cons, I believe. But this is also a change that I'm not very comfortable about without some investigation and checking with users. Hence, either the current behavior should be maintained, or we need to put the PR on hold until investigated OK. Conclusion
Let me know if I misunderstood the lost search path case. |
129f138 to
6559591
Compare
|
@eknabevcc I did consider the I tested it in the debugger (see screenshot) and it worked. |
6559591 to
30dadfe
Compare
|
I also added the .xodr loading part. Now only the .osgb part is missing. |
15f535c to
0f42fdd
Compare
|
I have also added the .osgb part now and updated some formatting. |
|
As stated in first comment my recommendation would be to do a baby step first, changing only the actual behavior according to the initial issue (i.e. remove only current directory as pivot search folder and keep C++14). Now we have instead extended changed behavior in addition to bumped C++ version. It's perfectly fine to keep the change like this, as one larger step. However then it needs some considerations and checking (both behavior and C++ version) before it can be merged. So we simply need to put in on hold until that can be done. Also before merge we should document the changed behavior. Preferably somewhere in the User guide. It's written in asciidoc format. Suggestion would be to edit it with Visual Studio Code. There is even an extension for previewing the result, AsciiDoc, although it's not perfect and usually not needed for simple text and lists. As complement I use the command line tool asciidoctor, from same developer, to generate HTML. I understand that you might want to postpone the documentation until we have a "go" for merge. But at least a brief description of the changed behavior is needed for the investigation. So maybe you could provide that in a simple text format here, the briefer the better. That could potentially be re-used for the User guide in a next step. Of course, I could handle the actual update in User guide if you'd prefer that, as long as you provide the content. Also, the commit don't pass the CI, failing on the compile step. That also needs to be looked into. |
|
Thanks for the preliminary review. Just to explain my thought process: I could add another non-C++ 17 pull request. But since I now understand the quirks of the current implementation and can avoid them as a user I am not in such a huge hurry to get it merged. You wrote "or we would need to hold this PR until we know the bump to c++ 17 is OK." and I decided aha that is an acceptable option for me, let's move forward. I definitely also did want to have a clean reference implementation using absolute and canonical loading paths for myself first to understand what is going on. All that absolute and canonical stuff is kind of hard to get right for all edge cases if you have to implement it yourself. People have long been using libraries for that. We can still do the step by step thing if you want though. It is just that I started with the last step and will now work my way back. I can then rebase this PR on top of the other if that is needed. A rough estimate of the C++ 17 transition roadmap would surely be helpful in deciding if it makes sense to put in the extra work. I can also edit the asciidoc documentation no problem. If you have some idea into which chapter this information needs to go let me know. As for the failing builds, it seems to be related to the tests which I did not run locally. Need to look into this and fix them. |
|
I'll get back regarding C++17 latest end of next week. Awaiting some input on the topic. Let's wait with further steps until then. Regarding failed CI: I just checked the first log file. Seems to be a compiler warning/error (we changed policy on CI to treat specified warnings as errors/failed build), so it could be a minor thing: |
Provide more feedback about the loading process of the interlinked files (.xodr, .osgb, catalogs) to the user and avoid loading files relativ to the working directory which may result in undesired side effects since the working directory can be located anywhere. The user may use the --path commandline option in order to intentionally emulate the old behavior.
0f42fdd to
17afd09
Compare
|
Alright, I will wait for an update. Take your time since it is not super critical. Meanwhile I took a stab at updating the documentation. What I am unsure about is how to run the tests locally. There also does not seem to be much information regarding that topic in the documentation. |
This is a preliminary pull request to potentially get some feedback. I yet have to add the same changes for other filetypes (not only the catalog files as it is now)
Provide more feedback about the loading process of the interlinked files (.xodr, .osgb, catalogs) to the user and avoid loading files relativ to the working directory which may result in undesired side effects since the working directory can be located anywhere. The user may use the --path commandline option in order to intentionally emulate the old behavior.