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

Remove duplicate entries for auto-completion suggestions and sort #70

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

martin-fleck-at
Copy link
Collaborator

  • Prefer package local names over global names
  • Sort local names before global name in textual and diagram editor

Fixes #66

Copy link

github-actions bot commented Oct 7, 2024

Unit Test Results

  3 files  ± 0   24 suites  +6   3m 3s ⏱️ +10s
 36 tests +11   36 ✅ +11  0 💤 ±0  0 ❌ ±0 
108 runs  +33  108 ✅ +33  0 💤 ±0  0 ❌ ±0 

Results for commit 84bfdb6. ± Comparison against base commit a38f61d.

♻️ This comment has been updated with latest results.

@martin-fleck-at martin-fleck-at force-pushed the feature/issue-66 branch 3 times, most recently from e6c6f8f to a319605 Compare October 8, 2024 17:32
- Prefer package local names over global names
- Sort local names before global name in textual and diagram editor

Fixes #66
@martin-fleck-at
Copy link
Collaborator Author

@harmen-xb I'm facing an issue with Windows on this one. Maybe you could help me out here since I don't have a Windows machine to try. It must have something to do with the paths not properly recognizing that one URI is not a child of another.

@harmen-xb
Copy link
Contributor

harmen-xb commented Oct 15, 2024

@martin-fleck-at I finally got the debugging to work (i had the same issue on linux when running a single test). It had to do with the 'outFiles' being set to an empty array apparantly.

I ran the unit tests of the uri-util class and I noticed a relavant difference in the posifxPath.relative function.

See the relative result in linux:
image

And on Windows:
image

See how in Windows the result is prefixed with ..\ i.s.o. only the child part. I have the idea the problem is in this part.

[UPDATE] I checked the uri-utils, what would happen if we only use nodePath and not the posix variant if it's available and then all tests on the uri-utils succeed on linux and on windows. Also I see the relative command working. I think this is because the unit tests only use posix paths, so there is no file:\C:\some\path as an example. It does however not solve the label issue (I can't find where the label is constructed in the code).

@harmen-xb
Copy link
Contributor

harmen-xb commented Oct 15, 2024

@martin-fleck-at

I looked at the "Completion for entity references in project B" scemario why it fails and I noticed in Windows the itemsToString function in langium-test.js returns a different result. The object is not prefixed with the system name, while in Linux it is.

Here in Linux:
image

And then same scenario in Windows:
image

When looking at it further I see the 'Label' property in Linux is prefixed with the system name, but in Windows it's not.

@martin-fleck-at
Copy link
Collaborator Author

martin-fleck-at commented Oct 17, 2024

[UPDATE] I checked the uri-utils, what would happen if we only use nodePath and not the posix variant if it's available and then all tests on the uri-utils succeed on linux and on windows. Also I see the relative command working. I think this is because the unit tests only use posix paths, so there is no file:\C:\some\path as an example. It does however not solve the label issue (I can't find where the label is constructed in the code).

That sounds good! So if I understand you correctly, not using the posix-path yields better result and for the test URIs we should use Windows-style paths on Windows and Unix-style paths on Unix and then it should work, is that correct? I do not really remember anymore why we had to do the posix-specific behavior tbh.

@harmen-xb
Copy link
Contributor

That sounds good! So if I understand you correctly, not using the posix-path yields better result and for the test URIs we should use Windows-style paths on Windows and Unix-style paths on Unix and then it should work, is that correct? I do not really remember anymore why we had to do the posix-specific behavior tbh.

The uri-utils tests do succeed on windows when you remove the usage of posix. But as I said, I think this is mainly because there is no test with a real Windows path starting with a drive. It might fail at that point, didn't have time to test this yet.

Maybe we can have a look at the code where the label of an object is constructed and debug it there. Cause on Windows it's stil not prefixed with the system node (like in Linux)..

Updated test utils to use posix join to force using forward slash.
@martin-fleck-at
Copy link
Collaborator Author

@harmen-xb The latest update made the CI happy so I think we can merge this, what do you think? Feel free to approve if you agree ;-)

Copy link
Contributor

@harmen-xb harmen-xb left a comment

Choose a reason for hiding this comment

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

Changes look good!

@harmen-xb harmen-xb merged commit 7b69226 into main Oct 18, 2024
5 checks passed
@harmen-xb harmen-xb deleted the feature/issue-66 branch October 18, 2024 15:23
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.

System diagram show entity lists double
2 participants