Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
List support: append method #1709
List support: append method #1709
Changes from 74 commits
9e8be09
82bffa9
a23c504
8ebf4c1
4d69c1a
e270e92
ea42345
87c69ed
0105fb0
004df3e
4d4a9a6
6a4cf9a
82c8eeb
d9b16be
cc0ace0
5472a59
7c3b2bc
3b3ec83
0e595d1
65fff84
832f2a6
6dbe191
561b01e
d76ebe3
31f65c2
5ea27e4
607e864
054c8e1
95d7fa4
1a9c2aa
8971fe3
0fe56f1
ab24e34
13d3348
9a95f60
20a134a
8e8de64
2dde3ce
fc6faff
d0b3286
6062a16
6a4e20f
4c17533
3ad7452
0a27c23
d24f595
7f26cc7
3421dfe
5c4ba6f
0cbeb0f
54cc2a4
b0a3369
d0cfeee
ad6fca5
85611b2
10e2294
1c221b9
44f18ba
89a402a
85ceef6
0197cc1
93d637a
8282f57
e628f6c
0f39b82
86ca222
a0337c3
67ad2fd
9db04e0
d984ec4
9b5daa8
09fa662
8767430
52addad
6d94603
eb9a747
c89bae3
ce7ec14
ed6e77c
c349a6e
c16295c
0772a46
2d23899
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 32 in pyccel/ast/class_defs.py
pyccel/ast/class_defs.py#L23-L32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code isn't tested. Please can you take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/bot accept
The final line is the only one without coverage. It was there before and is a fallback for strange function calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you check the refactored version please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did work in my linux machine 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 existinglanguage
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also possible if you have a suggestion for a good name? We may need up to 4 files:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, using four files is a bit cumbersome... We could call them
test_epyccel_lists.py
test_epyccel_lists__no_c.py
test_epyccel_lists__no_fortran.py
test_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.