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

Refactor serializers #220

Closed
wants to merge 4 commits into from
Closed

Refactor serializers #220

wants to merge 4 commits into from

Conversation

nickpfeiffer05
Copy link

Purpose

Lodash is unnecessary and can be replaced by native features. Serializer methods are better suited as static methods.

Tickets

https://github.com/orgs/sandboxnu/projects/20?pane=issue&itemId=80651976

Contributors

@Anzhuo-W , @nickpfeiffer05

Feature List

  1. Removed reliance on Lodash in all serializer files
    • The previous serializers used many Lodash functions (.pick, .groupBy, etc.), that can now be replaced easily by modern JS features
    • PR has replaced all old Lodash functions with native JS
  2. Conversion to static methods
    • Converted all serializer methods to static
    • Adjusted the use of serializers from instanced-calling to static-calling in searcher.ts

Reviewers

Primary:

@ananyaspatil

Secondary:

@mehallhm , @cherman23

Copy link
Contributor

@mehallhm mehallhm left a comment

Choose a reason for hiding this comment

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

Updating line 82 in the serializers/courseSerializer.ts breaks references in graphql/resolvers/class.ts. Updating the later to use the new static implementation of the method should clear the CI

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 80.613% (+0.1%) from 80.481%
when pulling a68d2b5 on refactor-serializers
into 33e48e4 on master.

Copy link
Contributor

@elvincheng3 elvincheng3 left a comment

Choose a reason for hiding this comment

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

Are there any existing tests for the serializers? If so, we should update/add to them for new code changes; if not, we should write tests.

Copy link
Contributor

@elvincheng3 elvincheng3 left a comment

Choose a reason for hiding this comment

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

Are there any existing tests for the serializers? If so, we should update/add to them for new code changes; if not, we should write tests.

@mehallhm mehallhm mentioned this pull request Oct 28, 2024
@mehallhm mehallhm closed this Nov 3, 2024
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.

5 participants