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

Ohm 1095 #1153

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Ohm 1095 #1153

wants to merge 6 commits into from

Conversation

Ken473
Copy link
Collaborator

@Ken473 Ken473 commented Oct 18, 2021

This is the first pass of the refactoring for the Channels and the clients. This is for you to see and comment if you're happy with the approach we have taken. We are going to add more refactorings and apply those to the rest of the entities.

@michaelloosen
Copy link
Contributor

Please fix linting and look at failing tests

Copy link
Contributor

@MattyJ007 MattyJ007 left a comment

Choose a reason for hiding this comment

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

@Ken473
Please copy over only the necessary logic changes into a new branch and open a new PR.
The changes here are convoluted and hidden within the linting - this is not acceptable practice. If large scale linting is required, it should be done as a separate commit. This makes logic changes easier to follow commit by commit instead of having to trawl through the 1450 lines of code you touched.

The formatting of the files is now also inconsistent with the project standard. Please use the linting tool included in the node dependencies - the command was included in the package.json - npm run lint:fix

You have also included committed multiple console.log statements throughout your code - an accidental commit occasionally is human - this level is sloppy. Please check the work you are adding with a diff tool or use git add -p to add your work in stages to prevent adding unnecessary lines.

@MattyJ007
Copy link
Contributor

Also, please include a message body and the ticket in your commit as well - not just a heading.
The body should explain why you are making the change. Poor commit messages make maintaining a codebase difficult - especially in a large scale API change.

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.

3 participants