codefori / vscode-rpgle

RPGLE language tools for VS Code
MIT License
39 stars 20 forks source link

Member resolves, IFS resolves and local `includePath` support #225

Closed worksofliam closed 1 year ago

worksofliam commented 1 year ago

Changes

Checklist

chrjorgensen commented 1 year ago

@worksofliam I was curious and went through your code for include and copy of members. And I noticed some things, that is not how the RPG compiler works (according to the documentation):

It seems like there's some confusion about this subject, probably because of different conventions and company standards. But IMHO we should follow the rules laid out by the RPG compiler. ➡️

worksofliam commented 1 year ago

@chrjorgensen wow, great write up!

  1. Whoops! Simple mistake on my part. I can correct to QRPGLESRC!
  2. We use rpgleinc just to give the member an extension. At resolve time, the extension isn't import, but we use it just to signify it is an include.
  3. I will fix up those IFS resolve inconsistencies.

Changes incoming over the next day or two with your suggestions. Thanks you!!

worksofliam commented 1 year ago

@chrjorgensen Based on your feedback, the only change I made is to use QRPGLESRC instead of QRPGLEREF.

The plan is to first get member resolves working, and then move onto IFS/local.

worksofliam commented 1 year ago

@chrjorgensen Further updates to improve the code and allow cross file system resolves and cleaner code. I updated the PR description to reflect all new changes.

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on 6178f1d3f2c5987976237b69534958ba4fc81f38.

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on 173bfb7cb83241b0f73f822e1ed602b26919b4b9.

worksofliam commented 1 year ago

@chrjorgensen

Changing the current library also didn't affect the resolve cache - I had to disconnect and reconnect to make the resolve work, after first having another current library than TESTPR225.

You can simple close and re-open the member/IFS/local file now.

I will post again when the remaining items are complete.

worksofliam commented 1 year ago

Great progress has been made.

image

@chrjorgensen Back to you for another review whenever you can.

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on 7dfe7f7b396d5ed9d16ace8b58e2021ee501f260.

chrjorgensen commented 1 year ago

@worksofliam IMHO the RPG /COPY documentation is confusingly organized - the INCLUDE FILES page seems the most important. It was probably added when RPG source code was allowed in the IFS.

An example that I was not really aware of: /COPY mylib/myfile,mymbr can mean

  1. in the QSYS filesystem library mylib with source file myfile and member mymbr OR
  2. in the IFS (root) filesystem subdirectory mylib in the current working directory with streamfile myfile,mymbr

The page above has more examples.

So both QSYS and IFS filesystem are searched by the compiler unless the filename is in quotes.

I won't even go into the IFS include path using INCDIR parameter or RPGINCDIR environment variable...!

chrjorgensen commented 1 year ago

One test still missing is /copy files in includePath - coming up later...

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on a11c031cdcb3d7a1a5949a0edb80899f92635044.

worksofliam commented 1 year ago
github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on f912aedb0ce12a9be8eee12d74fd73ff96eb8bd4.

worksofliam commented 1 year ago

@chrjorgensen I know you're about to depart for holiday, but whenever you're back...

✅ The copy of TESTPR225/SRCFIL,MBR3 no longer worked This is fixed. ✔️ The IFS copy of /dir1/dir2/file.rpg worked now. ✅ The IFS copy of /dir1/dir2/file worked - partially. Could only be resolved, when exact filename existed, but no seatch for suffixes .rpgle and .rpgleinc is performed. Left the current logic in until vscode-ibmi 2.0.0, which will allow us to search for many names. ✔️ The IFS copy of dir1/dir2/file.rpg worked now. ✅ I could not get the IFS copy of "ifs file containing blanks" to work. This is fixed. Share what your include directive looks like if it fails again. ✅ I could not get the IFS copy of 'ifs file containing blanks' to work. This is fixed. Share what your include directive looks like if it fails again.

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on 1ccde46f7091ac42bc2cc9038b5f4d486e9f3803.

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on 2be46c7aced0675f1f5a1feeb65d4656241e7e90.

worksofliam commented 1 year ago

Thanks for testing @chrjorgensen!

For 1, 2 & 3: Right now, when including a member we default to rpgleinc as the extension. Our APIs don't provide the extension (always simply returns .MBR) so I decided to use .rpgleinc for now. Adding an additional call to get the valid extension is expensive and didn't seem worthy for now. While this may confuse our developers, I think initially this is something we can start with.

For 5: I tested this and it seemed to work as you described:

image

For 7 & 8: I mentioned previously that this style of include (paths with spaces) won't be supported in this PR (a risk I am also willing to take). It looks like there is a bug in the memberResolve API that will need fixing before this can work. I don't want to hold up this PR. edit: I made a PR in vscode-ibmi and requested your review which, in theory, should solve this.

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on d8f74bc6eb78072ea39723db342d999e95bb76a9.

chrjorgensen commented 1 year ago

@worksofliam IMHO it would be better to have no extension for members - since the extension is not specified in the /COPY statement and the extension actually is not part of the member name.

Instead of showing billede it should just show /TESTPR225/QRPGLESRC/MBR1 - is this difficult to implement?

I have reviewed your PR #1482 and made a suggestion...

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on 49205295f4bb4a175e6ed4f38495e78304d1bd90.

worksofliam commented 1 year ago

@chrjorgensen 5 isn't working for you? That is odd! I actually wonder if we should take a look at that together before we merge this.

worksofliam commented 1 year ago

@chrjorgensen I went ahead and fixed the issue where the path name was being cut off as you discovered! Whenever you have the time, take another look with my latest commit!

github-actions[bot] commented 1 year ago

👋 A new build is available for this PR based on 4bb0894f59cb93a5d1170889246f446658ae13e1.