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

add --lang option to tutorial fixes issue #173 #174

Merged
merged 3 commits into from
Mar 20, 2021
Merged

add --lang option to tutorial fixes issue #173 #174

merged 3 commits into from
Mar 20, 2021

Conversation

fozy81
Copy link
Member

@fozy81 fozy81 commented Jan 13, 2021

Closes #173.

@ijlee2 ijlee2 requested a review from chancancode January 13, 2021 13:31
@ijlee2
Copy link
Member

ijlee2 commented Jan 13, 2021

@fozy81 Let's ask Godfrey (@chancancode) to help with reviewing the PR. I think there's a way to test the changes locally and Godfrey may be able to help better how you can do so.

fozy81 and others added 2 commits January 13, 2021 13:45
Co-authored-by: Isaac Lee <16869656+ijlee2@users.noreply.github.com>
@fozy81
Copy link
Member Author

fozy81 commented Jan 13, 2021

Thanks @ijlee2 I've included your suggestion and corrected slight typo improves improves. 👍

@chancancode
Copy link
Collaborator

thanks for the PR! I thought the original plan is to use the interactive mode for this, has that changed?

@fozy81
Copy link
Member Author

fozy81 commented Jan 13, 2021

Hi @chancancode - I'm new to this repo, how do I run the interactive mode?

@chancancode
Copy link
Collaborator

Sorry, I meant the interactive ember new ("wizard") mode in emberjs/rfcs#638 which was merged around the same time IIRC

@ijlee2
Copy link
Member

ijlee2 commented Jan 13, 2021

@chancancode Hmm, I'm not sure. I don't think I remember a plan to combine teaching --lang and the wizard. I found a mention about covering --lang in the tutorial in Core Notes, but wasn't able to find a mention about combining it with the wizard for the tutorial in the repository.

I think @fozy81 made the PR based only on the ember new --lang RFC, independently of the wizard RFC. (Please correct me if I'm mistaken.)

Would you recommend making a pull request that covers both at the same time for the tutorial? If so, maybe we can provide @fozy81 specifics on how we might do that.

@ijlee2
Copy link
Member

ijlee2 commented Jan 13, 2021

@fozy81 I found the Discord chat where Godfrey had explained to me how to check changes locally. I think these are the commands that you will want to run:

# Use Node 12.18 (version 14 resulted in an error for me) and install packages
nvm use 12.18.3
yarn install

# Build the dist directory
yarn build

@chancancode The yarn build resulted in the error,

Stack Trace and Error Report: /var/folders/2z/93zyyhx13rs879qr8rzyxrb40000gn/T/error.dump.a13c49292d2dbf43f2aeec1a83f7670c.log

    at ChildProcess.exithandler (child_process.js:303:12)
    at ChildProcess.emit (events.js:315:20)
    at maybeClose (internal/child_process.js:1021:16)
    at Socket.<anonymous> (internal/child_process.js:443:11)
    at Socket.emit (events.js:315:20)
    at Pipe.<anonymous> (net.js:674:12) {
  killed: false,
  code: 1,
  signal: null,
  cmd: 'ember new super-rentals --lang en --yarn',
  stdout: "The option '--lang' is not registered with the 'new' command. Run `ember new --help` for a list of supported options.\n" +
    'installing app\n' +
    'Ember CLI v3.19.0\n' +
    '\n' +
    'Creating a new Ember app in /Users/isaac/Desktop/super-rentals-tutorial/dist/code/super-rentals:\n' +
    'The globPattern "en" did not match any files, so no file updates will be made.\n' +
    'WARNING: Skipping npm install: package.json not found\n' +
    '\n' +
    'Initializing git repository.\n' +
    'Error creating new application. Removing generated directory `./super-rentals`\n',
  stderr: 'Command failed: git commit -m Initial Commit from Ember CLI v3.19.0\n' +

Do we need to be worried about this error?

I think the error may be because we are trying to run ember new with the --lang option, but the ember-cli version is 3.19 instead of 3.21+.

@chancancode
Copy link
Collaborator

Yes yarn build is right – but you would also need to set the MAPBOX_ACCESS_TOKEN env variable or else the later chapters would fail. See https://github.com/ember-learn/super-rentals-tutorial#how

The error you encountered is because of the lock file locked ember-cli locally. I meant to re-roll the dependencies so I'd do that now. In the meantime, you can temporarily work around the issue by changing "ember-cli": "*" to "ember-cli": "^3.21.0" in package.json locally and yarn install. Just don't commit the package.json and lock file changes in this PR.

@chancancode
Copy link
Collaborator

Hmm, I'm not sure. I don't think I remember a plan to combine teaching --lang and the wizard. I found a mention about covering --lang in the tutorial in Core Notes, but wasn't able to find a mention about combining it with the wizard for the tutorial in the repository.

The general idea is that we don't want to teach the users a lot of different flags that they are supposed to remember. ember new should Just Work™ and should be how most users, especially beginners trying ember for the first time, experiences the tool. Instead of having to pass the flag, it will interactively ask your preference (and offers to remember it and not ask again in the future, IIRC). The flags are there so that other scripts and automation or power users can use them, but I don't think we intend to bring that front-and-center for new users.

@fozy81
Copy link
Member Author

fozy81 commented Jan 13, 2021

@ijlee2 @chancancode Thanks for the useful background for running the super-rentals tutorial locally. I was proceeding on the --lang RFC and associated github issues. But I see the wizard RFC is interesting approach. Either way, I'll try to help out where I can.

@chancancode
Copy link
Collaborator

The error you encountered is because of the lock file locked ember-cli locally. I meant to re-roll the dependencies so I'd do that now. In the meantime, you can temporarily work around the issue by changing "ember-cli": "*" to "ember-cli": "^3.21.0" in package.json locally and yarn install. Just don't commit the package.json and lock file changes in this PR.

I re-rolled the dependencies in #175, so if you pull and rebase that issue should go away now.

@ijlee2 ijlee2 merged commit 5e6610d into ember-learn:master Mar 20, 2021
@chancancode
Copy link
Collaborator

@ijlee2 thanks for taking care of the PRs, do you have time to pair on “deploying” this during the week? It mostly just amounts to merging the PR in guides source but I just wanted to let you know about the issues with the images. Otherwise we can do this asynchronously too

@ijlee2
Copy link
Member

ijlee2 commented Mar 21, 2021

@chancancode Yep, definitely I'm up for pairing. I'm CET time zone so if we can meet around noon your time, that'd be good. Let's finalize the date and time through Discord?

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.

add support for --lang flag
3 participants