Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Jul 11, 2023

The record fields are no longer completed in

namespace Meh

module Module1 =
    type R1 =
        { Field1: int }

    type R2 =
        { Field2: int }

module Module2 =
    open Module1

    { R1.{caret} }

On removal of when not fieldsOnly this started working again.
I'm not familiar enough with the code to really say if this change is good or not.
@auduchinok this appears to be related to #12872.
Maybe this rings a bell for you.

Running as a draft first to see what CI will say.

@auduchinok
Copy link
Member

On removal of when not fieldsOnly this started working again.

Interesting. Let's see if there's any CI failure with this change.

@nojaf nojaf marked this pull request as ready for review July 11, 2023 13:28
@nojaf nojaf requested a review from a team as a code owner July 11, 2023 13:28
@psfinaki
Copy link
Contributor

@auduchinok so WDYT? I trust our tests (in that area) enough to let this in.


But generally I wonder what was the reason for the original condition there.

@auduchinok
Copy link
Member

auduchinok commented Jul 12, 2023

But generally I wonder what was the reason for the original condition there.

That change tried to capture both unqualified names from the current environment and record fields. The former provides types, values and other items, but fields aren't allowed to be unqualified, unless inside records (or some special cases, e.g. property initialization in an application). It also seems be about filtering non-fields items for subsequent field completion.
Completion did not work as expected for some cases like { } prior to that change, in some cases until you type a part of field/expression in, so there's some identifier inside.

There's chance a change like #14821 made that condition not needed (or could be the reason the { R. } case got broken), as I started working on this change prior to that PR was merged in. And it could be a bug. 🙂

@auduchinok so WDYT? I trust our tests (in that area) enough to let this in.

Yes, looks good to me! @nojaf has also checked that our tests are green with this change, so it seems good.

@nojaf It seems a few parts weren't tested in that PR, could you please check if these cases still work too?

@psfinaki
Copy link
Contributor

Thanks @auduchinok for a detailed answer!

Yep, more tests - more good, the suggested scenarios are also worth checking.

@psfinaki psfinaki enabled auto-merge (squash) July 12, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants