Field Names for AST union cases #1845
Conversation
src/fsharp/ast.fs
Outdated
src/fsharp/ast.fs
Outdated
There was a problem hiding this comment.
After looking at this I wonder if we can change the type name to Range with uppercase R
There was a problem hiding this comment.
it's the same as FCS this way
There was a problem hiding this comment.
Yes please align as closely as possible with FCS until we've iterated a round of merges
There was a problem hiding this comment.
@cloudRoutine Indeed I'd prefer it if it's 100% identical, it will make life hell otherwise.
There was a problem hiding this comment.
aren't the ast.fs files the same? can't this one replace the one in FCS?
There was a problem hiding this comment.
Yes in theory but we normally rely on Git to mark up conflicts when we do merges
Note we normally merge visualfsharp --> fsharp --> FCS.
Because we already allowed these additions to FCS then we will get conflicts when we merge a conflicting version of these changes into visualfsharp.
Basically every time we change FCS without first submitting to visualfsharp we build up lots of work for ourselves. It's useful from time to time (especially when engineering in this repo was hard and CI incomplete) but we pay a price for it.
There was a problem hiding this comment.
@cloudRoutine If you can do the integrations visualfsharp --> fsharp --> FCS then that would help, though it's not easy to do those.
You should first integrate all changes besides these ones from VF --> F -> FCS
Then make the precise aligning change to VF which gets the diff smaller
Then integrate again VF --> F --> FCS
Note the VF doesn't contain the commit history from F or FCS (and indeed can't contain those for various legal and technical reasons). So we can't integrate FCS --> VF or F --> VF. But F and FCS do contain the commit history from VF.
|
Ahh ok..... Before we add more field names to @cloudRoutine Could you merge these over please? Or are these changes already the same is those? |
|
@dsyme they're almost the same changes, I stopped using |
|
@cloudRoutine We really, really have to merge these repos... |
cloudRoutine
force-pushed
the
caseNames
branch
2 times, most recently
from
9d36155 to
32df5f1
Compare
src/fsharp/ast.fs
Outdated
There was a problem hiding this comment.
What are these bools for?
There was a problem hiding this comment.
They indicate (isLastCompiland, isExe) - last file in input, and are we treating the input as if it's defining a .EXE (no warning on missing "module" in last compiland)
There was a problem hiding this comment.
Messy, see e.g. let isLastCompiland = fst cenv.isLastCompiland and some isExe thing
|
@cloudRoutine I'm too heads down in VS2017 issue to answer these - just trawl through the code to track their use - probably language service stuff in vs... |
|
these are the ones I couldn't find when I went looking for them, I'll try again once the find references service is working properly |
|
@cloudRoutine Just do a textual search for the appropriate identifiers |
cloudRoutine
force-pushed
the
caseNames
branch
from
32df5f1 to
4fc2da0
Compare
cloudRoutine
force-pushed
the
caseNames
branch
from
4fc2da0 to
b19ffb6
Compare
|
@dsyme I took all the changes that were made in FCS applied them in one commit, then applied the additional names and changes in a second commit. Should I cherry pick these commits onto fsharp and FCS? or did I not do it right? |
Yes, if you make identical changes to fsharp and FCS then things should be oK. That file doesn't change much |
5 participants
Field Names for Union Cases improves the clarity of intellisense tooltips and makes pattern matching easier.