List support: append method #1709
Conversation
…to raise proper error message in case or dtype conflicts
Farouk-Echaref
added
Feature
|
Hello again! Thank you for this new pull request 🤩. Please begin by requesting your checklist using the command |
|
Here is your checklist. Please tick items off when you have completed them or determined that they are not necessary for this pull request:
|
Farouk-Echaref
force-pushed
the
List_Support/append_method
branch
from
f89aad4
to
cc0ace0
Compare
|
/bot commands |
|
This bot reacts to all comments which begin with
Beware: if you have never contributed to this repository and you are not a member of the Pyccel organisation, the bot will ignore all requests to run tests until permitted by a trusted reviewer. |
|
/bot show tests |
|
The following is a list of keywords which can be used to run tests. Tests in bold are run by pull requests when they are marked as ready for review:
These tests can be run with the command |
tests/errors/semantic/blocking/APPEND_LIST_INCOMPATIBLE_TYPES_3.py
Outdated
Show resolved
Hide resolved
tests/epyccel/test_epyccel_lists.py
Outdated
| @pytest.mark.parametrize( 'language', [ | ||
| pytest.param("c", marks = [ | ||
| pytest.mark.skip(reason="append() not implemented in c"), | ||
| pytest.mark.c]), | ||
| pytest.param("fortran", marks = [ | ||
| pytest.mark.skip(reason="append() not implemented in fortran"), | ||
| pytest.mark.fortran]), | ||
| pytest.param("python", marks = pytest.mark.python) | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Could we not define a language fixture in this file, rather than copying this long decorator many times?
There was a problem hiding this comment.
Is it possible to put xfail marks in a fixture? The xfail marks should be removed in a few weeks or months once the method is supported
There was a problem hiding this comment.
could you please explain how that would be possible? I was unable to do so since I'm not that familiar with writing test files using pytest so I did in the most basic way
There was a problem hiding this comment.
Is it possible to put xfail marks in a fixture? The xfail marks should be removed in a few weeks or months once the method is supported
It should be possible:
@pytest.fixture( params=[
pytest.param("fortran", marks = [
pytest.mark.skip(reason="list methods not implemented in fortran"),
pytest.mark.fortran]),
pytest.param("c", marks = [
pytest.mark.skip(reason="list methods not implemented in c"),
pytest.mark.c]),
pytest.param("python", marks = pytest.mark.python)
],
scope = "module"
)
def language(request):
return request.paramThere was a problem hiding this comment.
can you check the refactored version please
There was a problem hiding this comment.
It did work in my linux machine 🤞
There was a problem hiding this comment.
Looks good but I wonder if we should name this fixture something like language_python_ok. At some point we will have some tests in this file which can use the existing language fixture (without the xfails) and some which need this one (and maybe some that need one where only C or only Fortran fails)
There was a problem hiding this comment.
Good point. How about using multiple test files and move the tests from one to the other as they start passing?
There was a problem hiding this comment.
This is also possible if you have a suggestion for a good name? We may need up to 4 files:
- Passing in Python only
- Passing in C and Python
- Passing in Fortran and Python
- Passing in C, Fortran and Python
There was a problem hiding this comment.
Well, using four files is a bit cumbersome... We could call them
test_epyccel_lists.pytest_epyccel_lists__no_c.pytest_epyccel_lists__no_fortran.pytest_epyccel_lists__python_only.py
Another solution is using a single file, but providing a utility function which generates the correct decorators with minimum user input.
In any case I suggest that we tackle this in a future PR.
pyccel-bot
bot
removed
the
Ready_to_merge
|
@Farouk-Echaref, @yguclu has a few questions/comments about your code. Can you go through and see if you agree with them. If not go ahead and explain why. Once you've adressed all the comments let me know with |
…ist_methods/ refactor is_homogeneous condition
|
/bot run linux |
|
/bot run pr_tests |
![pyccel-bot[bot]](https://avatars.githubusercontent.com/in/364561?s=60&v=4){kind=small}
|
/bot show tests |
|
The following is a list of keywords which can be used to run tests. Tests in bold are run by pull requests when they are marked as ready for review:
These tests can be run with the command |
|
/bot run pyccel_lint |
pyccel-bot
bot
added
the
Ready_to_merge
Add support for the append() method of Python lists to the semantic stage. Add handling of that method to the Python printer. This fixes #1689.
Commit Summary
append()method, inheriting fromPyccelInternalFunction. The constructor ensures compatibility between the datatypes of theappend()argument and the list for homogenous lists.ClassDefrepresenting aNativeHomogeneousList. It includes a methodappend()implemented by theListAppendclass.`get_cls_base()function to properly check on the classtype of the containers._print_ListAppend()to generate the appropriate python representation of theappend()method.PythonList) is assigned to variables created via expressions such asa = [1]append()method involving many scenarios where the function could be used.