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.

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

Pythran file #647

Closed
wants to merge 2 commits into from
Closed

Conversation

diorcety
Copy link
Contributor

No description provided.

pythran/spec.py Outdated
.replace('#', '')
.replace('\_o< pythran >o_/', '#pythran'))
else:
pythran_data = "\n".join(['#pythran ' + str(a) for a in data.split("\n")])

Choose a reason for hiding this comment

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

you're not filtering out empty lines, so

foo(int)

foo(str)

will cause an error

pythran/spec.py Outdated
@@ -316,5 +319,5 @@ def specs_to_docstrings(specs, docstrings):
)


def spec_parser(path):
return SpecParser()(path)
def spec_parser(path, python_file=True):

Choose a reason for hiding this comment

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

I would feel more comfortable with a subclass of SpecParser that just overrides the right method, putting shared code where it's meant to be, instead of this ugly boolean

with open(path_or_text) as fd:
pythrancode = fd.read()
else:
pythrancode = path_or_text

Choose a reason for hiding this comment

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

if you do it here, maybe you can remove the file_or_path from the spec.py file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is to have the filepath inside spec.py in order to have the file path in the error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, shouldn't we replace non #pythran line by empty line in order to avoid wrong line information?

Choose a reason for hiding this comment

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

I'd say yes.

spec_file = os.path.join(dirname, module_name + ".pythran")
if os.path.isfile(spec_file):
from pythran.spec import spec_parser
specs = spec_parser(spec_file, False)

Choose a reason for hiding this comment

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

here, instead of the bool, you would use an aux_spec_parser that crates the deriving object etc

from pythran.spec import spec_parser
specs = spec_parser(spec_file, False)

output_file = compile_pythrancode(module_name, file_path, specs,
output_file=output_file,
cpponly=cpponly,
**kwargs)

Choose a reason for hiding this comment

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

I don't see a single test case :-)

Choose a reason for hiding this comment

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

still no test case;-)

@diorcety diorcety force-pushed the pythran_file branch 4 times, most recently from 629d9ef to e15ad97 Compare March 17, 2017 14:45
pythran/spec.py Outdated
.replace('#', ''))

SpecParser._parse(self, pythran_data)

Choose a reason for hiding this comment

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

I really prefer tis version \o/

@@ -343,18 +349,24 @@ def compile_pythranfile(file_path, output_file=None, module_name=None,
"""
if not output_file:
# derive module name from input file name
_, basename = os.path.split(file_path)
dirname, basename = os.path.split(file_path)

Choose a reason for hiding this comment

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

dirname is unsued later, so better keep _?

@diorcety diorcety force-pushed the pythran_file branch 3 times, most recently from 6868536 to 916b43b Compare March 20, 2017 14:48
@serge-sans-paille
Copy link
Owner

Moved to #656

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.

None yet

2 participants