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

Prism should read yml files too SO-200 #299

Merged
merged 5 commits into from
May 14, 2019
Merged

Prism should read yml files too SO-200 #299

merged 5 commits into from
May 14, 2019

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented May 14, 2019

  1. Correctly reveals yml files as yaml for graphite
  2. Handles startup exceptions and prints them out
  3. Updates the README

@XVincentX XVincentX marked this pull request as ready for review May 14, 2019 16:46
@XVincentX XVincentX changed the title Prism should read yml files too Prism should read yml files too SO-200 May 14, 2019
@@ -64,3 +66,8 @@ export class GraphFacade {
return compact(nodes.map<IHttpOperation>(node => node.data as IHttpOperation));
}
}

function transformLanguage(lang: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest a simple map over function:

import { Languages } from '@stoplight/graphite/graph';

const langMap = {
  json: Languages.Json,
  yaml: Languages.Yaml,
  yml: Languages.Yaml,
}

This way can also check that language exists in langMap and report error if it does not.

language = langMap[language];
if (!language) {
  // report error and stop or whatever
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah interesting idea, I didn't think of reusing that part of graphite!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marbemac how about importing the function that does that in graphite and use it?

import { filenameToLanguage } from '@stoplight/graphite/backends/filesystem/utils'


language: filenameToLanguage(path)

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better! Still suggest reporting an error if language is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The language will never be undefined; the function is using a proxied map returning the string you set in case it's not in the map

https://github.com/stoplightio/graphite/blob/master/src/backends/filesystem/utils.ts#L15-L26

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it - ok well then 🤷‍♂, hopefully folks don't feed prism files that are not json or yaml.

Copy link
Contributor

@marbemac marbemac left a comment

Choose a reason for hiding this comment

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

As long as you verified e2e this solves the problem, implementation looks good to me!

@XVincentX
Copy link
Contributor Author

@philsturgeon How about you follow the guide and try to release Prism?

@XVincentX XVincentX merged commit cbc96b2 into master May 14, 2019
@XVincentX XVincentX deleted the fix/yaml_yml branch May 14, 2019 17:50
@XVincentX XVincentX added this to the Prism v3 Release milestone May 25, 2019
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.

None yet

2 participants