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

fix: use denormalize theme name for server request #410

Merged
merged 1 commit into from Nov 25, 2020

Conversation

cukejianya
Copy link
Contributor

@cukejianya cukejianya commented Nov 6, 2020

When using the command resume server --theme [insert them name], the request got back a 400 error with the error message posted down below.

image

The PR creates a denormalizeTheme function in the builder 'index.js' file (mostly for readability) and uses that function to denormalize the theme name from 'jsonresume-theme-even' to 'even'.

@thomasdavis
Copy link
Member

@cukejianya Great work on the PR. This is to suppose both --theme jsonresume-theme-even and --theme even ?

@rbardini
Copy link
Contributor

Wouldn't it be better to handle this in the theme server instead?

@thomasdavis
Copy link
Member

@rbardini yeah thinking about it more, I'd rather people just use the api one way, which is to leave off jsonresume-theme-

And I think the resume users should do the same.

Unless someone has a good reason that they require this I will be closing for now.

@cukejianya
Copy link
Contributor Author

cukejianya commented Nov 25, 2020

@thomasdavis Hey, sorry for the late response. This PR is for a bug. It's not to add new functionality. Currently, the functionality you're talking about already exist. So when a user passes in --theme jsonresume-theme-even or --theme even, the cli currently normalizes that theme (regardless of how the user inputed it) as jsonresume-theme-even.

Because of that, when passing the string "jsonresume-theme-even" to the server, the API apparently concatenates that string to "jsonresume-theme-" resulting in the API looking for "jsonresume-theme-jsonresume-theme-even"

This PR is to denormalize the theme from jsonresume-theme-even to even, so when we POST it to the API, the API concatenates the string "even" to "jsonresume-theme-", thus finding the correct theme "jsonresume-theme-even"

@rbardini
Copy link
Contributor

Indeed, if we intend to use it with a single format (leaving off jsonresume-theme-), then this PR is relevant.

I was thinking the other way around: making the API flexible to accept both normalized (e.g. jsonresume-theme-even) and denormalized (e.g. even) themes, like the CLI does.

@thomasdavis thomasdavis reopened this Nov 25, 2020
@thomasdavis
Copy link
Member

Aha my bad, will merge the PR but keep the API less flexible.

Thanks @cukejianya

@thomasdavis thomasdavis changed the title fix(theme): use denormalize theme name for server request fix: use denormalize theme name for server request Nov 25, 2020
@thomasdavis thomasdavis merged commit d7e25fc into jsonresume:master Nov 25, 2020
@github-actions
Copy link

🎉 This PR is included in version 2.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants