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

Fix: remove quote from challenge where not needed [english] #35493

Conversation

Nitin96Bisht
Copy link
Contributor

@Nitin96Bisht Nitin96Bisht commented Mar 2, 2019

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • None of my changes are plagiarized from another source without proper attribution.
  • All the files I changed are in the same world language (for example: only English changes, or only Chinese changes, etc.)
  • My changes do not use shortened URLs or affiliate links.

Refer #35481

@RandellDawson
Copy link
Member

@Nitin96Bisht Did you not read @Manish-Giri's comment regarding letting a first-timer work on the issue? Always make sure you read the issue for any extra comments before creating your PRs.

@Nitin96Bisht
Copy link
Contributor Author

Nitin96Bisht commented Mar 2, 2019

@RandellDawson I read @Manish-Giri's comment. However, I had already made changes in my local repository before his comments. I will take care of this from now onwards.

@RandellDawson
Copy link
Member

Just because you make changes in your local repo, does not mean you have to create the PR.

@Nitin96Bisht
Copy link
Contributor Author

I understand I apologize for this. I will take care of it now. Kindly let me know If I should close it.

@RandellDawson
Copy link
Member

No, we will review it.

@RandellDawson
Copy link
Member

@Nitin96Bisht I did a quick search myself and you appear to have missed about 30+ instances throughout the english curriclum. What search pattern did you use?

@RandellDawson
Copy link
Member

For example, I found two instances in the Tests section of the following file:

curriculum/challenges/english/02-javascript-algorithms-and-data-structures/object-oriented-programmingunderstand-the-prototype-chain.english.md

where </code>") which should be: </code>

@camperbot camperbot added language: English scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. labels Mar 3, 2019
@Nitin96Bisht
Copy link
Contributor Author

@RandellDawson I used "<code> and </code>" as search pattern. You are right there are still many instances in test section. In case of:

curriculum/challenges/english/02-javascript-algorithms-and-data-structures/object-oriented-programmingunderstand-the-prototype-chain.english.md

There is a testString like testString: assert(/Object\.prototype\.isPrototypeOf/.test(code), "Your code should show that <code>Object.prototype</code> is the prototype of <code>Dog.prototype</code>"); where </code>" at the end is associated with "Your code string instead of "<code>. Same is with another files. Correct me if I am wrong. I think It does not need to be removed.

@RandellDawson
Copy link
Member

@Nitin96Bisht You can ignore anything in the assert message arguments, because those are not even used anymore by the code logic. The text above each textString is used instead. However, anything in the text is visible, so it should be fixed.

Unfortunately, your search pattern will yield a few false positives, so you will just have to manually review before replacing.

@Nitin96Bisht
Copy link
Contributor Author

Nitin96Bisht commented Mar 3, 2019

@RandellDawson Yes, you are right. I will check and try all possible cases before replacing.

Kindly check at your end I have updated all for /curriculum/challenges/english.

@Nitin96Bisht Nitin96Bisht force-pushed the fix/remove-quotes-from-challenge-where-not-needed branch from f1ba7c3 to 06a5bc5 Compare March 3, 2019 18:18
@RandellDawson RandellDawson self-assigned this Mar 3, 2019
@RandellDawson
Copy link
Member

@Nitin96Bisht It appears you have removed a quotation mark which was actually needed, because now a test is breaking. You will need to review the changes in your latest commit and make sure you are not changing anything in a testString section (which is most likely the issue).

Also, before pushing any more changes, make sure your code passes all the tests when running npm run test:curriculum locally. Otherwise, the build will just keep failing when you push the changes.

@Nitin96Bisht
Copy link
Contributor Author

@RandellDawson Thanks. Can you please help me in identifying the issue as I opened Test Details but I could not understand the root cause. It is the very first time my changes have failed. I will be glad If you could help me in this.

@RandellDawson RandellDawson force-pushed the fix/remove-quotes-from-challenge-where-not-needed branch from 06a5bc5 to e54692a Compare March 4, 2019 03:28
@RandellDawson
Copy link
Member

@Nitin96Bisht You just needed to rebase your local branch. I fixed it for you.

@Nitin96Bisht
Copy link
Contributor Author

Thanks @RandellDawson 👍 What about other languages? Should we create a new issue for all other languages and allow the first time contributor to work on it or you have any other plan.

@lynxlynxlynx
Copy link
Member

It's English that will have the most conflicts, not the other languages. But since it makes it easier to review, separate PRs for the other languages are better in this case (it's a recent general policy too).

Feel free to fix the others as well.

Copy link
Member

@RandellDawson RandellDawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Nitin96Bisht Once this PR is merged (one more approval needed), then you can create separate PRs for each language if you want with the same changes.

Thank you.

@thecodingaviator
Copy link
Contributor

Removed the closes tag since the PR actually does not close. It'll be closed when all languages are fixed

Copy link
Contributor

@thecodingaviator thecodingaviator left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@thecodingaviator thecodingaviator merged commit 228d873 into freeCodeCamp:master Mar 19, 2019
@thecodingaviator
Copy link
Contributor

Thanks @Nitin96Bisht . Go ahead and create PRs for other languages if you want to

@Nitin96Bisht Nitin96Bisht deleted the fix/remove-quotes-from-challenge-where-not-needed branch March 20, 2019 04:19
@Nitin96Bisht
Copy link
Contributor Author

@thecodingaviator I was looking for the Chinese and Spanish language but I could not find any such string <code>..<code>" expect testString. Please check from your side.

@thecodingaviator
Copy link
Contributor

@Nitin96Bisht This is from the Chinese folder:
image

You should also take note that you should change the text string mainly and not the testString because the challenges now mostly derive their challenge texts from the text and not the testString.

@Nitin96Bisht
Copy link
Contributor Author

Nitin96Bisht commented Mar 24, 2019

@thecodingaviator You are right, but I was saying all the search results are in testString not test. I think we should allow first timer to work on this once we verify from our end whether there exists any such text in text area or not.

@thecodingaviator
Copy link
Contributor

@Nitin96Bisht An &quot; evaluates to a quote too, so it is even in the tests

@roshni-b
Copy link

Hi, I am a first timer and would like to work this. Where should I start?

@Nitin96Bisht
Copy link
Contributor Author

@roshni-b You can start working on any of the languages except the English language. Kindly go through contribution guidelines. @RandellDawson Kindly let her know whether you or any one is working on this So that she can start. I am not working on this as of now because we should allow first timer to work on this.

@RandellDawson
Copy link
Member

@roshni-b No one else is working on the non-English languages, so go ahead. Just keep each on a separate PR.

@roshni-b
Copy link

roshni-b commented Apr 3, 2019

Alright thanks, I am working on Russian to start with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants