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

Disambiguate "Priority Queue" instructions #38805

Merged
merged 4 commits into from
Jul 15, 2020
Merged

Conversation

TyMick
Copy link
Contributor

@TyMick TyMick commented May 12, 2020

Checklist:

  • 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.
  • All the files I changed are in the same world language, for example: only English changes, or only Chinese changes, etc.

For future reference, should I have combined #38803, #38804, and this one into a single PR?

@gitpod-io
Copy link

gitpod-io bot commented May 12, 2020

@camperbot camperbot added language: English scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. labels May 13, 2020
@moT01 moT01 added the status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. label May 25, 2020
@moT01
Copy link
Member

moT01 commented May 25, 2020

I have blocked this for now. See the pinned code freeze issue

@raisedadead
Copy link
Member

Limitations removed on Coding Interview Prep Challenges.

@raisedadead raisedadead removed the status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. label May 27, 2020
@ojeytonwilliams
Copy link
Contributor

For future reference, should I have combined #38803, #38804, and this one into a single PR?

There's no hard and fast rule for PRs, but each commit should be 'atomic' in the sense that it should be reasonably small and easy to understand. In this case it seems better to have those separate, because one changes style, the other meaning. It would be easy to miss the typo fix in the middle of all the changes to indentation.

As for this PR, I see that there's a reference to front (i.e. peek, I assume), but that does not appear in the tests or solution. Could you remove references to it or add a test? Either is fine.

tinktink1
tinktink1 previously approved these changes Jun 4, 2020
Copy link

@tinktink1 tinktink1 left a comment

Choose a reason for hiding this comment

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

I think that the changed you made to the instructions clears up everything. It could have been confusing for people just learning and I think that this will help them out good job!

@TyMick
Copy link
Contributor Author

TyMick commented Jun 5, 2020

@ojeytonwilliams Added front() to the solution and a couple of tests along with it! Here's the prettified version of the second new test:

assert(
  (function () {
    var test = new PriorityQueue();
    test.enqueue(["David Brown", 2]);
    var front1 = text.front();
    test.enqueue(["Jon Snow", 1]);
    var front2 = test.front();
    test.dequeue();
    test.enqueue(["A", 3]);
    var front3 = test.front();
    test.enqueue(["B", 3]);
    test.enqueue(["C", 3]);
    test.dequeue();
    var front4 = test.front();
    return (
      front1 === "David Brown" &&
      front2 === "Jon Snow" &&
      front3 === "David Brown" &&
      front4 === "A"
    );
  })()
);

@moT01 moT01 closed this Jul 8, 2020
@moT01 moT01 reopened this Jul 8, 2020
@gitpod-io
Copy link

gitpod-io bot commented Jul 8, 2020

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.

@tywmick One of your test's testStrings had a typo which caused the test to fail. Please correct and test your code locally by changing to the curriculum directory and running the following command:

npm run test -- -g "Create a Priority Queue Class"

Once you have passed all the tests successfully, you can make a new commit with the fixed code.

Thank you.

@RandellDawson RandellDawson added the status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP label Jul 8, 2020
Co-authored-by: Randell Dawson <5313213+RandellDawson@users.noreply.github.com>
@TyMick
Copy link
Contributor Author

TyMick commented Jul 9, 2020

Whoops, thanks for catching that, @RandellDawson! Tests passing now 👌🏼 Thanks for your help!

@RandellDawson RandellDawson removed the status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP label Jul 9, 2020
@RandellDawson
Copy link
Member

LGTM @moT01 @ojeytonwilliams Merge if you agree.

Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

LGTM @tywmick 🎉 Thanks for contributing to freeCodeCamp 🎉

I have added this solution to the guide as well 😄

@moT01 moT01 merged commit cd90da1 into freeCodeCamp:master Jul 15, 2020
@TyMick TyMick deleted the patch-6 branch July 15, 2020 16:50
abbathaw pushed a commit to abbathaw/freeCodeCamp that referenced this pull request Jul 24, 2020
* Disambiguate "Priority Queue" instructions

* Add front() to solution

* Add tests for front() method

* Fix test typo

Co-authored-by: Randell Dawson <5313213+RandellDawson@users.noreply.github.com>

Co-authored-by: Randell Dawson <5313213+RandellDawson@users.noreply.github.com>
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

7 participants