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

Added an option to select a custom context to match moment-timezone l… #35

Merged
merged 1 commit into from Jan 31, 2021

Conversation

diniciacci
Copy link
Contributor

…ocation. Fixes #32.

Copy link
Owner

@gilmoreorless gilmoreorless left a comment

Choose a reason for hiding this comment

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

Thanks! I gave this a quick manual test and it worked as expected.

I'll take a look at the missing test. If I can't get something working quickly, I'll put out a release as it is now, then do a bigger rework later.

Comment on lines -177 to +186
// Invalid cache dir (not a valid path)
if ('cacheDir' in options) {
try {
path.parse(options.cacheDir);
} catch (error) {
throwInvalid(`Provided cacheDir is an invalid path: '${options.cacheDir}'`);
// Check options that require a valid path
['cacheDir'].forEach(option => {
if (option in options) {
try {
path.parse(options[option]);
} catch (error) {
throwInvalid(`Provided ${option} is an invalid path: '${options[option]}'`);
}
}
}
});
Copy link
Owner

Choose a reason for hiding this comment

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

I like this change! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, your code inspired me ;)

@@ -31,7 +31,7 @@ describe('instantiation', () => {
);
});

// TOOD: Warn instead of throw?
// TODO: Warn instead of throw?
Copy link
Owner

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry for this 😅. Please don't be ashamed, I do much worse. I fixed it just because else code analysis doesn't pick it up and never warns that there is a TODO somewhere to work on. Everything is great!

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I don't mind at all. I'm just amazed that I spent so much time looking at that code over the years and never spotted it! 😆

@gilmoreorless gilmoreorless merged commit 9d8f3bb into gilmoreorless:master Jan 31, 2021
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.

Plugin not working when moment-timezone is not in 'node_modules' directory
2 participants