-
Notifications
You must be signed in to change notification settings - Fork 379
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(theme): fix serving and exporting local themes #537
Conversation
88f4262
to
ac94e1a
Compare
@@ -5,9 +5,6 @@ describe('renderHTML', () => { | |||
const originalRequireResolve = require.resolve; | |||
const mockThemePath = 'mock/path/to/jsonresume-theme-even'; | |||
require.resolve = (...args) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While writing the new tests, I found that this mock of resolve is not called.
So tests are running against the actual file system.
Possibly the mock can be removed. Or maybe it needs to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to write that as a TODO comment instead of leaving as an PR comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR comment is an implicit question about what the correct action is.
Any TODO would come from the answer to the question.
And I'm not sure that in code TODOs is the way this project manages future code changes. They may prefer creating a GitHub issue.
@@ -3,7 +3,7 @@ const themeServer = | |||
const fs = require('fs'); | |||
const request = require('superagent'); | |||
const chalk = require('chalk'); | |||
const renderHtml = require('./render-html'); | |||
const renderHtml = require('./render-html').default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project uses a mix of CommonJS and ES6 modules. This should probably be made consistent to prevent bugs like this.
I won't address it in this PR.
This solution works. thank you. |
@@ -43,5 +40,20 @@ describe('renderHTML', () => { | |||
'<!doctype html>', | |||
); | |||
}); | |||
|
|||
it('should reject theme with invalid path', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to what we see, currently, we don't test the theme with valid path, I wonder if that can be tested as well: some test which would be red without your alterations and green after them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test below ('with local theme path') is a valid path.
It would fail pre my changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I've missed that one. Looks good!
Can i kwon will it merge soon ? |
bump |
Are there any project maintainers about to review/comment on this PR? I see that there haven't been any merges for a few months. |
Is this project abandoned? |
I'm getting It would be great to get this merged. I'll ping the top commiters, so those who have commit rights and some free time can take a look: @rolandnsharp @thomasdavis @PeterDaveHello @rbardini @erming @antialias Also if someone has time to run Thanks all for this project! |
Gentle ping @rolandnsharp @thomasdavis @PeterDaveHello @rbardini @erming @antialias |
I don't have problem with local theme. So, for clear and small PR. Should we split to two PR? |
So now @paskal has approved this PR what is the next step? |
I am no one here. Someone from the list above needs to approve and merge it. |
🎉 This PR is included in version 3.0.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks @XuluWarrior for fixing this, and sorry everyone for the delay! As a side note, I've been writing an alternative JSON Resume builder from scratch that I hope might shape the future of resume-cli, in case anyone wants to give it a try 🙂 |
Issue with not loading local themes is now fixed jsonresume/resume-cli#537
Issue with not loading local themes is now fixed jsonresume/resume-cli#537
Issue with not loading local themes is now fixed jsonresume/resume-cli#537
Issue with not loading local themes is now fixed jsonresume/resume-cli#537
This reverts commit 16eae28.
)"" This reverts commit b24a94b.
Fixes #492, fixes #538.