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: Test Lint #28

Merged
merged 6 commits into from
Dec 9, 2023
Merged

Feat: Test Lint #28

merged 6 commits into from
Dec 9, 2023

Conversation

CodeWithEmad
Copy link
Member

Assorted testing and linting improvements:

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

In addition:

  • Typing added for various functions

PS. I must admit that the "Typing" part could use some improvement.

@CodeWithEmad CodeWithEmad mentioned this pull request Nov 24, 2023
Closed
@CodeWithEmad
Copy link
Member Author

@regisb this failed becaue MyPy needs to have types-requests installed here. Since we decided that all dev dependencies should be installed from requirements/dev.txt in the tutor, is it okay to open a PR there for types-requests?

@regisb
Copy link
Collaborator

regisb commented Nov 24, 2023

Let's not add types-requests to tutor core if it's not strictly necessary. Can you implement the [dev] requirements here, and add types-requests to them, such that we can write in the CI:

pip install -e .[dev]

instead of:

pip install 'tutor[dev]>=16.0.0,<17.0.0'

I think this should solve our issue, right?

regisb
regisb reviewed Nov 24, 2023
View reviewed changes
tutorxqueue/plugin.py Outdated Show resolved Hide resolved
regisb
regisb reviewed Nov 24, 2023
View reviewed changes
tutorxqueue/plugin.py Outdated Show resolved Hide resolved
@CodeWithEmad CodeWithEmad requested a review from regisb November 24, 2023 19:57
regisb
regisb reviewed Nov 27, 2023
View reviewed changes
setup.py Outdated Show resolved Hide resolved
regisb
regisb reviewed Nov 27, 2023
View reviewed changes
setup.py Outdated Show resolved Hide resolved
@regisb regisb requested a review from jfavellar90 November 27, 2023 09:14
@CodeWithEmad
feat: typing added
de9a761
@CodeWithEmad
feat: Added 'dev' requirements to 'extras_require' in setup
255a130 
These include 'tutor' (version 16.x) for development and 'types-requests' (version 2.31.0.0). This addition helps in managing package dependencies during development.
jfavellar90
jfavellar90 reviewed Nov 27, 2023
View reviewed changes
tutorxqueue/plugin.py
Comment on lines +154 to +160
(
submission_id,
submission_key,
grade,
correct,
message,
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CodeWithEmad what is the purpose of grouping these 5 variables in the print_result method?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to fix the typing issue where *args: tuple[Any, ...]. It may not be the optimal solution, but I couldn't find any other alternative.

@jfavellar90
Copy link
Collaborator

@CodeWithEmad thanks for your valuable contribution! There's just one comment to be addressed from my side. For the rest, it looks good to me :)

CodeWithEmad
CodeWithEmad commented Dec 8, 2023
View reviewed changes
Copy link
Member Author

@CodeWithEmad CodeWithEmad left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Oh! My comments were pending for 2 week! I hate it when Github does that!

setup.py Outdated
Comment on lines 37 to 40
install_requires=[f"tutor{TUTOR_VERSION}" , "requests"],
extras_require={
"dev": [
f"tutor[dev]{TUTOR_VERSION}" ,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of this but I didn't want to repeat myself, so I moved the TUTOR_VERSION. let's decide a general form for this, so I can set dev for all plugins.

Copy link
Member Author

Choose a reason for hiding this comment

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

after this was merged, should I add a development section for all plugins (and cookiecutter) ?

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tutorxqueue/plugin.py
Comment on lines +154 to +160
(
submission_id,
submission_key,
grade,
correct,
message,
),
Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to fix the typing issue where *args: tuple[Any, ...]. It may not be the optimal solution, but I couldn't find any other alternative.

@regisb
Copy link
Collaborator

regisb commented Dec 8, 2023

Can we merge this today? I'd like to merge as many PRs as possible before Quince, on Monday.

@regisb regisb merged commit 8c2f78a into overhangio:master Dec 9, 2023
1 check passed
@CodeWithEmad CodeWithEmad deleted the feat/test-lint branch December 9, 2023 18:52
regisb added a commit that referenced this pull request Dec 11, 2023
@regisb
fix: add missing pkg-config package (#26)
18a6d5b 
Build fails in nightly with the following error:

	#28 5.503   × Getting requirements to build wheel did not run
	    successfully.
	#28 5.503   │ exit code: 1
	#28 5.503   ╰─> [24 lines of output]
	#28 5.503       /bin/sh: 1: pkg-config: not found
	#28 5.503       /bin/sh: 1: pkg-config: not found
	#28 5.503       Trying pkg-config --exists mysqlclient
	#28 5.503       Command 'pkg-config --exists mysqlclient' returned
	    non-zero exit status 127.
	#28 5.503       Trying pkg-config --exists mariadb
	#28 5.503       Command 'pkg-config --exists mariadb' returned non-zero
	    exit status 127.
	#28 5.503       Traceback (most recent call last):
	#28 5.503         File
	    "/openedx/venv/lib/python3.8/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py",
	line 353, in <module>
	#28 5.503           main()
	#28 5.503         File
	    "/openedx/venv/lib/python3.8/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py",
	line 335, in main
	#28 5.503           json_out['return_val'] =
	    hook(**hook_input['kwargs'])
	#28 5.503         File
	    "/openedx/venv/lib/python3.8/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py",
	line 118, in get_requires_for_build_wheel
	#28 5.503           return hook(config_settings)
	#28 5.503         File
	    "/tmp/pip-build-env-58chejnv/overlay/lib/python3.8/site-packages/setuptools/build_meta.py",
	line 355, in get_requires_for_build_wheel
	#28 5.503           return self._get_build_requires(config_settings,
	    requirements=['wheel'])
	#28 5.503         File
	    "/tmp/pip-build-env-58chejnv/overlay/lib/python3.8/site-packages/setuptools/build_meta.py",
	line 325, in _get_build_requires
	#28 5.503           self.run_setup()
	#28 5.503         File
	    "/tmp/pip-build-env-58chejnv/overlay/lib/python3.8/site-packages/setuptools/build_meta.py",
	line 341, in run_setup
	#28 5.503           exec(code, locals())
	#28 5.503         File "<string>", line 154, in <module>
	#28 5.503         File "<string>", line 48, in get_config_posix
	#28 5.503         File "<string>", line 27, in find_package_name
	#28 5.503       Exception: Can not find valid pkg-config name.
	#28 5.503       Specify MYSQLCLIENT_CFLAGS and MYSQLCLIENT_LDFLAGS env
	    vars manuallY
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfavellar90 jfavellar90 jfavellar90 left review comments

@regisb regisb regisb left review comments

Assignees

@jfavellar90 jfavellar90

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 @regisb @jfavellar90