Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

go.mod: allow no space before comment, do not allow double slash in unqouted string #2424

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

calebdoxsey
Copy link
Contributor

I believe this will fix the issues identified in #2423 .

A space is no longer required after the directive, so both

require// this is now a comment

and

require(

should be matched.

Comments will also be detected within unquoted strings:

    module k8s.io/kubernetes// this is now a comment

Which I believe replicates this line from the parser: https://github.com/golang/go/blob/master/src/cmd/go/internal/modfile/read.go#L546.

Finally anything unmatched is marked as invalid.

@ramya-rao-a
Copy link
Contributor

Thanks @calebdoxsey!

@brainsnail thoughts?

@brainsnail
Copy link
Contributor

Just loaded it up and tested out @calebdoxsey changes. Looks good to me regarding the comment formatting after the keywords.

I'm not super sure about the formatting for unmatched/invalid lines though.

Here's an example where I just entered a line break prior to a matched ( -
image

The invalid or unmatched highlighting is a nice addition though once that tweak is made.

Thanks again, @calebdoxsey!

@calebdoxsey
Copy link
Contributor Author

@brainsnail I believe newlines are not allowed after the require keyword. It errors out:

go: errors parsing go.mod:

So I think the highlighting matches the behavior of the go mod parser.

@marco-m
Copy link

marco-m commented Jun 4, 2019

hello @ramya-rao-a would you have time to have a look?

@ramya-rao-a ramya-rao-a merged commit 0bb096e into microsoft:master Jun 5, 2019
@marco-m
Copy link

marco-m commented Jun 5, 2019

thanks!

@calebdoxsey
Copy link
Contributor Author

Thanks @ramya-rao-a

@calebdoxsey calebdoxsey deleted the issue2423 branch June 5, 2019 14:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants