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

Correcting typeorm scripts - Issue #204 #206

Merged
merged 2 commits into from
Sep 7, 2018
Merged

Correcting typeorm scripts - Issue #204 #206

merged 2 commits into from
Sep 7, 2018

Conversation

Jrfidellis
Copy link
Contributor

@Jrfidellis Jrfidellis commented Sep 6, 2018

Issue

Fix #204.

Solution and steps

  • Remove ts-node dependency.
  • Update the migration commands.

Checklist

  • Add/update/check docs.
  • Add/update/check tests.
  • Update/check the cli generators.

@LoicPoullain
Copy link
Member

Hi @Jrfidellis ,

Thanks for submitting this PR. I added some comments in the description to improve its traceability.

The examples package is just a place I use to do some manual tests. It does not contain the templates used to generate the app when running the command foal createapp. These templates are actually located in the cli package. Here’s how it is organized:

src
'- generate # Handles the commands `foal createapp` and `foal generate`
  |- generators # Contains the code which renders the templates or updates the files
  |- mocks # Contains some pieces of code used to test the file "updaters"
  |- specs # Defines how the generated files should look like in different scenarios (specifications)
  |- templates # Contains the actual templates used to generate the files
  '- utils # Contains some helpers shared by all the generators

Updating the examples package is therefore necessary but we also need to update the package.json files located in specs and templates. Once done you can run the generators tests with the command npm run dev:test:generators at the path packages/cli (they will be executed in the Github hook anyway).

If you encounter problems let me know! 😉

@LoicPoullain LoicPoullain changed the base branch from master to 0-6-0-beta-4 September 6, 2018 16:00
@LoicPoullain LoicPoullain mentioned this pull request Sep 6, 2018
10 tasks
@LoicPoullain LoicPoullain self-requested a review September 6, 2018 16:01
@Jrfidellis
Copy link
Contributor Author

Jrfidellis commented Sep 6, 2018

@LoicPoullain, i wasn't able to run the tests, maybe because of the Issue #187?

All the errors (39 in total) look like this one:

 34) createService when the directory / exists should render the authenticator templates in the proper directory.:

      AssertionError [ERR_ASSERTION]: 'export { BarFoo } from \'./bar-foo.service\';\r\nexport { TestFooBar } from \'./test-foo-bar.service\';\n' === 'export { BarFoo } from \'./bar-foo.service\';\r\nexport { TestFooBar } from \'./test-foo-bar.service\';\r\n'
      + expected - actual

       export { BarFoo } from './bar-foo.service';
      -export { TestFooBar } from './test-foo-bar.service';
      +export { TestFooBar } from './test-foo-bar.service';

Copy link
Member

@LoicPoullain LoicPoullain left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@LoicPoullain
Copy link
Member

Hmm, ok so the tests are running properly in the travis build anyway: https://travis-ci.org/FoalTS/foal/builds/425373591?utm_source=github_status&utm_medium=notification. So the PR is good.

I guess that your issue is related to how Windows and Unix systems (Mac OS, Linux) handle differently line breaks. Depending on the OS sometimes they have different encodings: \n, \r or \r\n.

What makes me think of that is that the two compared strings look exactly the same but they actually end with a different character: \n and \r\n.

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

Successfully merging this pull request may close these issues.

3 participants