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.

Sign up for GitHub

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

Jump to bottom

Feat: Testing and Linting #16

Merged
merged 6 commits into from
Nov 24, 2023
Merged

Feat: Testing and Linting #16

merged 6 commits into from
Nov 24, 2023

Conversation

CodeWithEmad
Copy link
Member

@CodeWithEmad CodeWithEmad commented Nov 18, 2023 •
edited
Loading

Assorted testing and linting improvements:

  • Makefile with test and lint targets
  • Long lines reformated with black
  • Sort imports with isort

In addition:

  • All configs moved to 1 place to better support the unification of plugins
  • Typing added for various functions

PS. Looks like Actions are not enabled on this repository.

@CodeWithEmad
Copy link
Member Author

@regisb we talked earlier about removing .pre-commit-config.yaml, .editorconfig, and linter.sh since these are not a part of the cookie-cutter. should I remove them as a part of cleanup in this PR?

@CodeWithEmad CodeWithEmad mentioned this pull request Nov 18, 2023
Closed
@regisb regisb requested a review from Faraz32123 November 21, 2023 07:50
@Faraz32123
Copy link
Collaborator

Hi @CodeWithEmad, thanks for creating a PR. I will look into it.
Can u please resolve the merge conflicts?

@regisb
Copy link
Collaborator

regisb commented Nov 21, 2023

@CodeWithEmad yes I think we should get rid of these files.

@CodeWithEmad
Copy link
Member Author

and what about .gitlab-ci.yml?

@regisb
Copy link
Collaborator

regisb commented Nov 21, 2023

We very much need that one! That's because our private CI runs on Gitlab.

@CodeWithEmad
Copy link
Member Author

Oh, that's good to know!

Faraz32123
Faraz32123 reviewed Nov 23, 2023
View reviewed changes
tutorcredentials/plugin.py Outdated
"OAUTH2_KEY_SSO": "credentials-key-sso",
"OAUTH2_KEY_SSO_DEV": "credentials-key-sso-dev",
"PLATFORM_NAME": "{{ PLATFORM_NAME }}",
"PRIVACY_POLICY_URL": "{{ LMS_HOST }}/privacy-policy",
Copy link
Collaborator

Choose a reason for hiding this comment

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

"{% if ENABLE_HTTPS %}https{% else %}http{% endif %}://{{ LMS_HOST }}/privacy-policy"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this PR needs to be rebased on top of master. @CodeWithEmad you seem to be missing some of the latest changes. Sorry!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes @regisb, you are right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I missed that one.

I think that this PR needs to be rebased on top of master

It's Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually PRIVACY_POLICY_URL was removed in the latest code of main branch.
Can u please rebase it again or sync it with master as PRIVACY_POLICY_URL is still in the PR.
Thanks for bearing it with me 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

It's Done.

Faraz32123
Faraz32123 reviewed Nov 23, 2023
View reviewed changes
Copy link
Collaborator

@Faraz32123 Faraz32123 left a comment

Choose a reason for hiding this comment

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

LGTM! with some changes requested.

@CodeWithEmad CodeWithEmad force-pushed the feat/test-lint branch 2 times, most recently from a9ba784 to 4ef31a6 Compare November 23, 2023 17:30
Faraz32123
Faraz32123 approved these changes Nov 24, 2023
View reviewed changes
Copy link
Collaborator

@Faraz32123 Faraz32123 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Faraz32123 Faraz32123 merged commit 70d0957 into overhangio:main Nov 24, 2023
@CodeWithEmad CodeWithEmad deleted the feat/test-lint branch November 24, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Faraz32123 Faraz32123 Faraz32123 approved these changes

@regisb regisb regisb left review comments

Assignees

@Faraz32123 Faraz32123

Labels
None yet
Projects
Tutor project management
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CodeWithEmad @Faraz32123 @regisb