Skip to content

fix regex for manifest route #5219

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 9, 2022
Merged

fix regex for manifest route #5219

merged 2 commits into from
Aug 9, 2022

Conversation

marshall-orevillestudios
Copy link

I noticed that the regex would match other strings than manifest.json such as manifest-json or something-manifest.json

@marshall-orevillestudios marshall-orevillestudios requested a review from a team May 23, 2022 15:03
@jsjoeio
Copy link
Contributor

jsjoeio commented May 23, 2022

I noticed that the regex would match other strings than manifest.json such as manifest-json or something-manifest.json

I'm guessing that wasn't intentional but let's double check with @code-asher.

@codecov
Copy link

codecov bot commented May 23, 2022 •
edited
Loading

Codecov Report

Merging #5219 (9f95fff) into main (04ff8c3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5219   +/-   ##
=======================================
  Coverage   72.42%   72.42%           
=======================================
  Files          30       30           
  Lines        1672     1672           
  Branches      366      366           
=======================================
  Hits         1211     1211           
  Misses        398      398           
  Partials       63       63
Impacted Files Coverage Δ
src/node/routes/vscode.ts 83.33% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04ff8c3...9f95fff. Read the comment docs.

@code-asher
Copy link
Member

code-asher commented May 23, 2022 •
edited
Loading

I think this was because there used to be a webmanifest.json link in addition to manifest.json? But as far as I can tell everything is manifest.json now. Could we get rid of the regex entirely and just do /manifest.json?

@jsjoeio jsjoeio added this to the May 2022 milestone May 23, 2022
jsjoeio
jsjoeio previously requested changes May 23, 2022
View reviewed changes
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind addressing @code-asher's feedback? Then we can merge this

@jsjoeio jsjoeio modified the milestones: June 2022, July 2022 Jun 28, 2022
@jsjoeio jsjoeio force-pushed the fix-regex-for-manifest-route branch from f130c78 to aaa4324 Compare August 8, 2022 22:38
@jsjoeio jsjoeio requested a review from code-asher August 8, 2022 22:43
@jsjoeio jsjoeio dismissed their stale review August 8, 2022 22:43

because i made a change

@jsjoeio jsjoeio enabled auto-merge (squash) August 8, 2022 22:43
@jsjoeio
Copy link
Contributor

jsjoeio commented Aug 8, 2022

@code-asher ready for review

@code-asher code-asher disabled auto-merge August 9, 2022 17:53
@code-asher code-asher merged commit efb5bae into coder:main Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@code-asher code-asher code-asher approved these changes

@jsjoeio jsjoeio jsjoeio left review comments

Assignees

@code-asher code-asher

Labels
None yet
Projects
None yet
Milestone
v4.5.2
Development

Successfully merging this pull request may close these issues.
3 participants
@marshall-orevillestudios @jsjoeio @code-asher