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

chore: Migrate admin,account to ES6 in app/controllers #3716

Merged
merged 4 commits into from
Dec 22, 2019
Merged

chore: Migrate admin,account to ES6 in app/controllers #3716

merged 4 commits into from
Dec 22, 2019

Conversation

kushthedude
Copy link
Member

Refers #3618

Short description of what this resolves:

  • Migrating the Controllers of Admin,Account section to ES6 syntax

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@kushthedude
Copy link
Member Author

@iamareebjamal Tested complete admin section with all function.
Please have a look and merge.

@kushthedude kushthedude changed the title Migration admin,account to ES6 in app/controllers feat: Migration admin,account to ES6 in app/controllers Dec 22, 2019
@auto-label auto-label bot added the feature label Dec 22, 2019
@kushthedude kushthedude changed the title feat: Migration admin,account to ES6 in app/controllers chore: Migration admin,account to ES6 in app/controllers Dec 22, 2019
@auto-label auto-label bot added chore and removed feature labels Dec 22, 2019
@iamareebjamal
Copy link
Member

Why not using codemods to automatically do this change rather than manually doing this repetitive task?

@iamareebjamal iamareebjamal changed the title chore: Migration admin,account to ES6 in app/controllers chore: Migrate admin,account to ES6 in app/controllers Dec 22, 2019
@iamareebjamal iamareebjamal merged commit 9b136d7 into fossasia:development Dec 22, 2019
@kushthedude
Copy link
Member Author

kushthedude commented Dec 22, 2019 via email

@iamareebjamal
Copy link
Member

I'm talking about from ember-cli-update

@kushthedude
Copy link
Member Author

kushthedude commented Dec 22, 2019 via email

@kushthedude
Copy link
Member Author

@iamareebjamal The following happens:

  • CLI-Update codemods does not work on babel parser and is an experimental feature which runs only ondecorators-legacy.
  • Also, The codemods always skips the main files :

Screenshot 2019-12-22 at 10 57 18 PM

Also, the codemods changed the hbs files rather than going to js, I don't know why:-
Screenshot 2019-12-22 at 10 59 25 PM

Hence doing it manually.

@kushthedude kushthedude deleted the es branch December 22, 2019 17:30
@iamareebjamal
Copy link
Member

For me, it changed the js files and hbs files both. hbs files need to be changed as well to glimmer syntax for octane

@kushthedude
Copy link
Member Author

For me, it changed the js files and hbs files both. hbs files need to be changed as well to glimmer syntax for octane

Can you please push the js files it changes?

@iamareebjamal
Copy link
Member

It was a quick test. But basically converts old syntax to new ES6 syntax, getters, decorators etc

@kushthedude
Copy link
Member Author

It was a quick test. But basically converts old syntax to new ES6 syntax, getters, decorators etc

It just changes the js files in tests directory and none others

@iamareebjamal
Copy link
Member

Not true

@iamareebjamal
Copy link
Member

I ran

ember update
ember update --run-codemods

@kushthedude
Copy link
Member Author

ember update --run-codemods

Let me try I was doing with ember-cli-update as I installed it globally

@iamareebjamal
Copy link
Member

That command is installed with ember-cli-update only

@kushthedude
Copy link
Member Author

That command is installed with ember-cli-update only

Again not successful, Also it broke the complete build.

@iamareebjamal
Copy link
Member

it broke the complete build

That is expected

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

Successfully merging this pull request may close these issues.

2 participants