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

Automatic Dropdown support for single Atom types does not work #7468

Closed
jdunkerley opened this issue Aug 2, 2023 · 9 comments · Fixed by #7670
Closed

Automatic Dropdown support for single Atom types does not work #7468

jdunkerley opened this issue Aug 2, 2023 · 9 comments · Fixed by #7670
Assignees
Labels
-language-server p-high Should be completed in the next sprint
Milestone

Comments

@jdunkerley
Copy link
Member

Any dropdown being chosen which is an Atom only type (e.g. Report_Unmatched) results in an incorrect import being added.

The import causes ambiguous import errors (as then both the module and type are imported).

Can be worked around by custom dropdowns but this should be fixed.

@hubertp
Copy link
Collaborator

hubertp commented Aug 2, 2023

@Frizi is that you area? Maybe you can let us know if something is missing from LS support?

@Frizi
Copy link
Contributor

Frizi commented Aug 3, 2023

I'm not sure what's the expected and observed result here. For me, selecting Report Unmatched from a dropdown doesn't result in any errors. I've checked that union has no override, and the default static tag value is used.

Zrzut ekranu 2023-08-03 123520

The generated code looks like this:

from Standard.Table.Data import Report_Unmatched
...
operator6 = operator8.union [operator8] keep_unmatched_columns=Report_Unmatched.Report_Unmatched

Is this incorrect?

@4e6 4e6 added the s-info-needed Status: more information needed from submitter label Aug 7, 2023
@4e6 4e6 removed the triage label Aug 7, 2023
@farmaazon
Copy link
Contributor

@jdunkerley ^ could you post here an exact scenario where "ambiguous import" happens?

@jdunkerley
Copy link
Member Author

image

The additional import is creating a clash with Report_Unmatched included in from Standard.Table import all.

  • The type Standard.Table.Data.Report_Unmatched.Report_Unmatched is exported by Standard.Table.
  • The added import brings in the module Standard.Table.Data.Report_Unmatched causing am abiguous import.

We should just put the type in as the chosen option (i.e. just Report_Unmatched here not Report_Unmatched.Report_Unmatched) and detect that this is already imported.

The issue with Nothing.Nothing bug is exactly the same as this case.

@jdunkerley jdunkerley assigned Frizi and vitvakatu and unassigned jdunkerley Aug 14, 2023
@jdunkerley jdunkerley removed the s-info-needed Status: more information needed from submitter label Aug 14, 2023
@vitvakatu vitvakatu moved this from ❓New to 📤 Backlog in Issues Board Aug 14, 2023
@vitvakatu vitvakatu moved this from 📤 Backlog to 🔧 Implementation in Issues Board Aug 21, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 22, 2023

Ilya Bogdanov reports a new STANDUP for today (2023-08-22):

Progress: Investigated the code and decided what needs to be done. Implemented a draft solution for choosing the correct expression. A proper solution is complicated because of suggestion database restrictions. Also finished the implementation of pasting nodes as plain text with the help of Pawel. It should be finished by 2023-08-24.

Next Day: Next day I will be working on the same task. continue working on the draft solution, investigating alternative implementations for the suggestion database.

@enso-bot
Copy link

enso-bot bot commented Aug 24, 2023

Ilya Bogdanov reports a new STANDUP for yesterday (2023-08-23):

Progress: Further investigation, added the code for exporting suggestion database in JSON for analysis. It should be finished by 2023-08-24.

@enso-bot
Copy link

enso-bot bot commented Aug 24, 2023

Ilya Bogdanov reports a new STANDUP for today (2023-08-24):

Progress: Investigating found the specific reason for the issue and a way to fix it. Considering different solutions. Also faced a peculiar bug when the lookup by the qualified name of the entry fails, even though the entry is present in the database. Found some issues in the hierarchy index implementation as well. It should be finished by 2023-08-24.

@vitvakatu vitvakatu moved this from 🔧 Implementation to 👁️ Code review in Issues Board Aug 28, 2023
@jdunkerley jdunkerley added this to the Beta Release milestone Aug 30, 2023
@vitvakatu vitvakatu moved this from 👁️ Code review to 🌟 Q/A review in Issues Board Aug 30, 2023
@farmaazon farmaazon moved this from 🌟 Q/A review to 🟢 Accepted in Issues Board Sep 5, 2023
@mergify mergify bot closed this as completed in #7670 Sep 5, 2023
mergify bot pushed a commit that referenced this issue Sep 5, 2023
Fixes #7468

The fix is pretty simple: we reuse the existing functionality for importing stuff and generating expressions. It fixes issues with `Nothing` or `Report_Unmatched` types.


https://github.com/enso-org/enso/assets/6566674/4e7addf9-2175-4f2a-a571-4ef823de5cb0

While debugging, I found it easier to work with a suggestion database when exported to some external format. Hence, I implemented serde serialization support for database entries and also a new debug shortcut <kbd>ctrl</kbd>+<kbd>shift</kbd>+<kbd>u</kbd> to dump all entries to the console.
@enso-bot
Copy link

enso-bot bot commented Sep 11, 2023

Ilya Bogdanov reports a new 🔴 DELAY for the provided date (2023-08-25):

Summary: There is 5 days delay in implementation of the Automatic Dropdown support for single Atom types does not work (#7468) task.
It will cause 1 day delay for the delivery of this weekly plan.

2 days are weekends

Delay Cause: The investigation took more time than initially estimated, mostly because I wasn't feeling confident with all the intricacies of the suggestion database. Also, we're short on people this week, so reviews and QAs are delayed.

@enso-bot
Copy link

enso-bot bot commented Sep 11, 2023

Ilya Bogdanov reports a new STANDUP for the provided date (2023-08-25):

Progress: With the help of Pawel, I investigated further and found the real issue. My clever tricks with the hierarchy index are not necessary. Instead, I need to refactor the code. Working on the solution. Also addressed review comments in copy-pasting nodes. It should be finished by 2023-08-29.

@farmaazon farmaazon moved this from 🟢 Accepted to 🗄️ Archived in Issues Board Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-language-server p-high Should be completed in the next sprint
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants