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

Update emmet #39

Closed
wants to merge 5 commits into from
Closed

Update emmet #39

wants to merge 5 commits into from

Conversation

rzhao271
Copy link
Contributor

See PR #33 for details.
This PR attempts to get that PR into a mergeable state by ensuring all tests pass.

@rzhao271
Copy link
Contributor Author

@devanshj feel free to review and comment if you have any thoughts

@devanshj
Copy link
Contributor

FWIW #33 was kinda a quick attempt to make update emmet without much documentation available as such, so this kinda soley relies on the tests and my assumptions and I'm pretty sure it's not reliable enough to be merged unless the original authors verify if it is.

@rzhao271
Copy link
Contributor Author

The original authors are in other teams or orgs now so I'm not really sure about contacting them.
Locally testing the change out, it seems fine to me, and my custom snippets still work. I haven't really tried different settings for the extension itself, though. I also have to figure out how to run the emmet integration tests on the vscode repo side.

The hardest part is probably making sure the configs continue to work afterwards, though I expect that even in the case of regressions, there should be more issues resolved than new issues opened after this PR is merged.

@devanshj
Copy link
Contributor

The original authors are in other teams or orgs now so I'm not really sure about contacting them.

Yes I'm aware, I wasn't exactly suggesting to reach out to them. I kinda meant that in a hypothetical sense that one way to have least risk on this PR is to have original authors review. To have least risk or be fine with low/whatever risk is upto vscode ofc :)

I also have to figure out how to run the emmet integration tests on the vscode repo side.

You might already know this but just in case it helps in anyway: there is already a configuration in launch.json, there is also one for extension tests in the vscode repo's lauch.json. I hope those are what you meant by integration tests.

there should be more issues resolved than new issues opened after this PR is merged

Yep I too think that will be the case hopefully

Also probably someone should look into this too (from #33 description):

Realized this doesn't take care of syntax profiles and also variables and it's not in the tests so missed out on that

@devanshj
Copy link
Contributor

devanshj commented Oct 16, 2020

As I mentioned here that I was working on getting rid of vscode-emmet-helper altogether (which I abandoned xD), I just pushed branch named emmet2 having some of wip, it has some good stuff (like this ig) that I don't remember xD. Letting you know in case it's helpful.

@rzhao271
Copy link
Contributor Author

Closing in favour of PR #41

@rzhao271 rzhao271 closed this Oct 21, 2020
@rzhao271 rzhao271 deleted the raymondzhao/update-emmet branch October 21, 2020 19:33
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.

2 participants