Skip to content

Prevent 'comment-your-javascript-code' from passing incorrectly due to multi-line comments which are not visible to users#12888

Merged
raisedadead merged 1 commit intofreeCodeCamp:stagingfrom
richiethomas:fix/multiline-comments
Jan 29, 2017
Merged

Prevent 'comment-your-javascript-code' from passing incorrectly due to multi-line comments which are not visible to users#12888
raisedadead merged 1 commit intofreeCodeCamp:stagingfrom
richiethomas:fix/multiline-comments

Conversation

@richiethomas
Copy link
Copy Markdown

@richiethomas richiethomas commented Jan 26, 2017

Pre-Submission Checklist

  • Your pull request targets the staging branch of freeCodeCamp.
  • Branch starts with either fix/, feature/, or translate/ (e.g. fix/signin-issue)
  • You have only one commit (if not, squash them into one commit).
  • All new and existing tests pass the command npm test. Use git commit --amend to amend any fixes.

Type of Change

  • Small bug fix (non-breaking change which fixes an issue)

Checklist:

Description

…o multi-line comments which are not visible to users
@BerkeleyTrue BerkeleyTrue added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Jan 26, 2017
@Greenheart Greenheart self-requested a review January 28, 2017 17:42
@no-stack-dub-sack
Copy link
Copy Markdown
Member

no-stack-dub-sack commented Jan 28, 2017

@richiethomas The new RegEx looks good and I pulled this down and ran it locally and the tests pass now only when they're supposed to, however - and this problem seems to have existed prior to your changes as well - if someone were to not close the multi-line comment, i.e. /* khkdflkhsldf, FCC kicks back Something went wrong. Please try again later. instead of an error message...

The linter throws a warning so there's that. I'm not sure if we just have to accept this or if there is some way around it. What do you think @Greenheart?

Otherwise LGTM!

Copy link
Copy Markdown
Member

@Greenheart Greenheart left a comment

Choose a reason for hiding this comment

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

@richiethomas @petercollingridge Thanks for reporting and solving this issue! The challenge now works as expected when I test locally 😄

@no-stack-dub-sack Regarding the Something went wrong-message, I only see one alternative at the moment.

Before the commit of this PR, there was a test checking for any comment that was closed (which always passed since it probably found some /*fcc*/-comment). This kind of test (or one checking for an matching number of /* and */) could theoretically solve this, but I think campers will realize something is wrong when they see the yellow warning sign from the linter in the editor.

I'd say we merge this 😊

@raisedadead raisedadead merged commit 311b517 into freeCodeCamp:staging Jan 29, 2017
@raisedadead raisedadead removed the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Jan 29, 2017
@systimotic
Copy link
Copy Markdown
Member

@no-stack-dub-sack @Greenheart The wrong error message displaying is a known issue for any syntax error, not just comments (#12733)

@richiethomas richiethomas deleted the fix/multiline-comments branch January 29, 2017 22:03
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.

6 participants