Fix228

[RobAnthony01]

Add \/\s? to start of looked for code and \s?\/ after semi colon. This traps any extra non-whitespace between the comments. Also changed the solutions to fit the new regex by adding comments /**/ in the solutions

Description

The tests could still pass if additional text was put between the comments on the solution. This checks to make sure ONLY whitespace and the correct solution is between the comments. Pre-Submission Checklist

• [x] Your pull request targets the dev branch.
• [x] Branch starts with either fix/, feature/, or translate/ (e.g. fix/challenge-tests)
• [x] All new and existing tests pass the command npm test.
• [ ] Use npm run commit to generate a conventional commit message. Learn more here: https://conventionalcommits.org/#why-use-conventional-commits
• [ ] The changes were done locally on your machine and NOT GitHub web interface. If they were done on the web interface you have ensured that you are creating conventional commit messages.

Checklist:

• [x] Tested changes locally.
• [x] Addressed currently open issue (replace XXXXX with an issue no in next line)

Closes #228

[joshalling]

@RobAnthony01 I don't know if we want to get so specific in our tests that we are requiring a comment in the solution. In the future, if the challenge changes and no longer has that comment at the start then campers will be unable to pass the challenge and not understand why.

This is why the testString should only reflect what the text is specifically asking for. The point of the issue that I reported was to ensure that the declaration was in the correct rule.

[RobAnthony01]

@joshalling I understand your disquiet. I felt uncomfortable making the change but reasoned it was the lesser of two evils. The comments are already there and specify that the solution should lie between the two comments. All i am doing is enforcing that to prevent any incorrect code (or junk!) still allowing the solution to pass. It's up to others to decide whether my solution is worse than the problem it's trying to solve!

[joshalling]

@RobAnthony01 I get where you are coming from, but in my mind this is just one of the caveats of using regex. We can't prevent all these edge cases unless we start going down a road where we are forcing the user to write their css in the format that we want regardless of whether it works or not. At that point, I think we are starting to get away from the core purpose of the challenge, which is to communicate a single point and then test that the point was understood.

I'm sorry if I come across as opinionated, but I just want us to really think about a few things before we make a change like this.

1. This sets a new precedent for all of the challenges and forces us to start asking questions like "Should we now implement something like this in every challenge that uses regex for tests?" or "Should we make it even more restrictive to prevent someone from writing random text outside of the comment?"
2. This potentially makes maintaining the code base more difficult because now if someone is changing one of these challenges, then they have to know that a comment must be included in the contents of the challenge.

I just think that if we start going down this road, then this could become a hindrance for the user rather than a benefit, because in the end, we just want to teach the user coding principles and make it as easy as possible to learn those principles.

At the very least, if we do decide to go with this change, then I think that the text of the tests should be updated to point out that the user's code must be inside of the comments.

[RobAnthony01]

@joshalling You don't come across as opinionated, just someone with an opinion. That's fine. I do agree this requires more thought. All I was trying to do was solve the issue you highlighted and, it may be the solution I suggest is not a good one (though it would solve the problem). The comments do say 'Please put your solution between these comments', so, as a user, I would not be upset if my solution fails because I didn't. We either need to accept that users can write whatever they like IN ADDITION to the solution and it will still pass (as is the case now) OR we will fail solutions which do not add the required solution and NOTHING ELSE. However, as you point out, nether what we have currently, nor what I suggest, would stop someone editing the code outside the solution area and making it nonsensical. I don't mind if this pull request is declined or accepted but it does require some consideration about what is the best way to approach this which is in the spirit of FreeCodeCamp.

[scissorsneedfoodtoo]

@RobAnthony01 and @joshalling, sorry for the delay. Just had a chance to look at this. Really appreciate the conversation that's happening here and the points being made on both sides.

I went through some of the tests in other sections and found a method that would work well with these challenges. @RobAnthony01, would you mind updating them to something similar to the following? This is for the "Create a Column Gap Using grid-column-gap" challenge specifically, but the others should be close:

{
  "text":
    "<code>container</code> class should have a <code>grid-column-gap</code> property that has the value of <code>20px</code>.",
  "testString":
    "assert($('.container').css('grid-column-gap') === '20px', '<code>container</code> class should have a <code>grid-column-gap</code> property that has the value of <code>20px</code>.');"
}

This way we should be able to check specific elements for the exact properties and values we want to test. Not sure if it'll work with all the challenges you changed here, but I imagine it'll work for most.

[joshalling]

@scissorsneedfoodtoo There is one potential problem with that solution, and it's that the solutions would all have to be removed as jQuery isn't included in the test suite. For that reason, it might be better to try to stick with regex. Another possible solution would be to do something like this:

.container\s*?{[^}]*\s+display\s*?:\s*?grid\s*?;[\s\S]*}

That only makes two changes to the current tests.

[^}]: rather than allowing any character here, this allows any character except a closing curly brace. This ensures that the user"s code is in the .container rule. \s+: adding this before the display: grid; declaration ensures that there is at least one space before the users's code so they cant pass something like junkdisplay: grid; and have it pass.

This solution does still pass if the user types junk display: grid;, but I think this is as good as we are going to get if we still want to keep these tests in the test suite.

Edit: We could also do something like this [;|\/]\s+ which would prevent junk display: grid; from passing, but it would still pass withjunk / display: grid;`. At that point, I think we are focusing too much on edge cases though.

[RobAnthony01]

@scissorsneedfoodtoo I would be happy to make the changes but, as @joshalling points out, we would have to remove the solutions altogether. Is that better or worse than adding the requirement that the solution is between the comments?

Also @joshalling, with your new solution, we would need to add spaces into all the solutions as a space is now required. I would argue that this would be less obvious as to WHY we were doing it (as opposed to the comments) and MORE LIKELY to be changed by future editors. If your argument against adding the comment requirement was that you didn't want to alter the solutions to have 'special characters' in them, this does not help. It also does not fix the whole problem!

[joshalling]

@RobAnthony01 Your right. We would need to change all the solutions that way too. I thought that there were already spaces there. The only reason I suggested that is because all the css in the challenges is already formatted with line returns, and it is the standard practice to write your css that way to make it more readable.

My initial suggestion when I reported the issue was to just add [^}] to make sure that the declaration was in the correct rule. I like this solution because it doesn't prevent any valid answers from passing. As @RobAnthony01 pointed out, the code in my comment above will not pass if you have all your css in one line without spaces, even though that is valid css. I was just looking for a compromise.

Ultimately, we need to decide if it's more important to make sure that all valid code passes or to make sure there isn't any invalid code that does pass.

Either that, or we should just use jQuery as @scissorsneedfoodtoo and remove the solutions.

[scissorsneedfoodtoo]

@joshalling, that's a good point! You're right that using jQuery would mean that the solutions won't pass and would have to be removed.

This is a bit of a tough one, since it would be great to have the valid solutions listed if possible. But looking at the SASS challenges it seems like many of the challenges use jQuery for the tests and don't have solutions. It would be nice to have tests that don't rely so heavily on regex since they can get tricky with all of the edge cases and future maintenance. What do you think, @raisedadead? Is having a solution for these CSS challenges important?