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

Issue #962 - Implemented B22 test #1433

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

Butenkite
Copy link

Description

B22 test implemented and passes. Added expected data for B22.

Collaborators:
@AAshGray
@bellxalli
@mlarsen-source

Partly addresses issue #962

Type of change

(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

(Describe any issues that remain or work that should still be done.)

@Butenkite
Copy link
Author

Butenkite commented Feb 27, 2025

The Conflict stems from the fact that the comments

// Implement test B17 - B22

were wrapped in test B16. We had unwrapped the rest of the comments along side the B22 comment and completed B22.

@Chocopepero
Copy link
Contributor

Thank you to @AAshGray @bellxalli @mlarsen-source for their work for not only implementing the test case as well as fixing the comment placement! All contributors have signed the CLA and the test case itself looks good. However there are a few changes that we ask be made.

@@ -174,7 +175,78 @@ mocha.describe('readings API', () => {
// Add B21 here

// Add B22 here
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the comment of your test case

@@ -174,7 +175,78 @@ mocha.describe('readings API', () => {
// Add B21 here

// Add B22 here
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete the lines between the function and the comment for B21. Leave just one between the comment and your test case.

@Butenkite
Copy link
Author

The styling has been updated so it now says:

// Add B20 here

// Add B21 here

[code code code]

It should be up to standard now. Let us know if anything else needs adjustment!

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.

2 participants