Skip to content

Add tests to each component#21

Open
biankaiwen111 wants to merge 35 commits intoFHDA:masterfrom
biankaiwen111:master
Open

Add tests to each component#21
biankaiwen111 wants to merge 35 commits intoFHDA:masterfrom
biankaiwen111:master

Conversation

@biankaiwen111
Copy link
Copy Markdown
Member

No description provided.

Comment thread tests/test_lab.json Outdated
Comment thread tests/06_from_raw_to_list_test.py Outdated
Comment thread tests/05_read_lab_time_test.py Outdated
Comment thread tests/04_read_course_proto_test.py Outdated
Comment thread src/FHDAlogger.py Outdated
Comment thread src/InsertData.py Outdated
Comment thread tests/05_read_lab_time_test.py Outdated
Comment thread tests/06_from_raw_to_list_test.py Outdated
Comment thread tests/test_lab.json Outdated
Comment thread tests/05_read_lab_time_test.py
@yifeili98
Copy link
Copy Markdown
Member

Current codeclimate issues could be ignored and will be resolved by .codeclimate.yml

@yifeili98
Copy link
Copy Markdown
Member

we may want to use an env file to define absolute path to src folder and log folder

hk-mp5a3
hk-mp5a3 previously approved these changes Jun 18, 2020
Copy link
Copy Markdown

@hk-mp5a3 hk-mp5a3 left a comment

Choose a reason for hiding this comment

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

LGTM, approved.
If it is possible, try to make the change(PR) as small as possible. It would be more readable for code reviewers to review your code.

@yifeili98
Copy link
Copy Markdown
Member

yifeili98 commented Jun 18, 2020

Review is temporary on hold until dotenv is implemented.

@kis87988
Copy link
Copy Markdown
Member

I should be should be able to put all *_pb2.py to a folder.. not sure yet.

Copy link
Copy Markdown
Member

@kis87988 kis87988 left a comment

Choose a reason for hiding this comment

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

must of part LGTM.

this PR needs to be rebased since that is password in the first commit#775d74a

and let's get start with dotenv implementation

Comment thread src/FHDAlogger.py Outdated
Comment thread src/InsertData.py Outdated
Comment thread src/InsertData.py Outdated
@biankaiwen111 biankaiwen111 requested a review from kis87988 May 12, 2021 05:23
Copy link
Copy Markdown
Member

@yifeili98 yifeili98 left a comment

Choose a reason for hiding this comment

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

Please git rm src/FHDAlogger.py, src/__init__.py, src/course_pb2.py, src/department_pb2.py and src/instructor_pb2.py

@biankaiwen111 biankaiwen111 requested a review from yifeili98 May 18, 2021 04:54
yifeili98
yifeili98 previously approved these changes May 18, 2021
Copy link
Copy Markdown
Member

@yifeili98 yifeili98 left a comment

Choose a reason for hiding this comment

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

LGTM

Sorry for another merge conflict🤪

Copy link
Copy Markdown
Member

@yifeili98 yifeili98 left a comment

Choose a reason for hiding this comment

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

LGTM, but you could add more descriptions to let reviewers know what features/refactors you have done in this PR.

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