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

Update generator js #6

Merged
merged 54 commits into from
Jul 5, 2016
Merged

Update generator js #6

merged 54 commits into from
Jul 5, 2016

Conversation

napiergit
Copy link

Hi @rcrichton, can you please review and comment on any additions changes you would like to see in the mediator scaffolding. Thanks in advance.

@rcrichton
Copy link
Member

@napiergit thanks this is looking good and definitely made my life easier. Some initial comments from building my new mediator:

  • Perhaps we should add a default .gitignore file?
  • The mediator config come out incorrect, in configDefs there is a parameter routePath however in the config object it is listed as just path. This causes mediator registration to fail.
  • I'm wondering if we should remove the openhim module as this won't always be needed (not sure about this)
  • If we remove the module above then the test will be empty, maybe we should add a sample test just to show people how tests should be written.

I should have more comments as I dig in, however, what do you think about these so far.

@napiergit
Copy link
Author

@rcrichton, thanks for the feedback, will add the .gitignore and fix the config object. I like the idea of a sample app with sample tests. I'm not too sure about the openhim module either (if it is really useful code we could always add it to the openhim-mediator-utils)

"mediatorRegister":true,

"enablePPA":false,
"ppaUsername":"rg-crichton"
Copy link
Member

Choose a reason for hiding this comment

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

Change this default to openhie

@rcrichton
Copy link
Member

Hey @napiergit in my new mediator I've changed some of the code to be inline with some of my comments here. Maybe you want to use that as a reference to help the changes here? https://github.com/jembi/openhim-mediator-basicauth-map

@rcrichton
Copy link
Member

@napiergit quick comment while trying this out. The chalk module isn't in the package.json so it doesn't get installed.

@rcrichton
Copy link
Member

Also .nyc_output should be in the .gitignore

function setupApp () {
const app = express()

app.all(mediatorConfig.endpoints[0].path, (req, res) => {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry one last change. I think maybe should do:

app.all('*', (req, res) => {

Then the mediator listens on all paths. We can then remove the config for the mediatorPath as it's no longer needed (and may confuse the user) and delete the path property from the default channel config which sets it to rather forward the path that it receives rather than set its own. I think that simplifies things. I changed my new mediator to do that.

@rcrichton
Copy link
Member

Thanks @napiergit this looks great. Just 3 more comments then this can be merged in 👍

I'm off to Rwanda on Sunday so if you get to this once I've gone, then maybe ask Hannes to do the final review, but once those 3 things are done, I think it's good to go. So may not need a full review.

@napiergit
Copy link
Author

Hey @hnnesv, can you please review this when you have a chance as Ryan is afk.

@@ -0,0 +1,3 @@
# <%= appName %>

This is a yeoman generated mediator to get you started on mediator development. To better usnderstand the types of mediators [click here](http://openhim.readthedocs.io/en/latest/user-guide/mediators.html#mediator-types).
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: understand

@hnnesv
Copy link
Contributor

hnnesv commented Jul 4, 2016

Thanks @napiergit - it's looking good! Just some minor comments from my side.

@napiergit
Copy link
Author

Hey @hnnesv, I have made the changes you asked for, anything else?

@hnnesv
Copy link
Contributor

hnnesv commented Jul 5, 2016

Nope, looks good to me! 👍

@hnnesv hnnesv merged commit 2483cfc into master Jul 5, 2016
@hnnesv hnnesv deleted the update-generator-js branch July 5, 2016 06:44
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

3 participants