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

Improve error message for [build] structure errors #890

Merged
merged 3 commits into from Apr 28, 2023

Conversation

perazz
Copy link
Contributor

@perazz perazz commented Apr 24, 2023

Address #889.
See discussion on the Fortran Discourse.

Solution: return package name on the error message triggered when an invalid [build] structure is detected.

John @urbanjost, can I ask you to give me a review of this change? (and edit the branch if you think this needs to be improved further). Thank you in advance.

@perazz perazz requested a review from urbanjost April 24, 2023 20:18
Copy link
Contributor

@urbanjost urbanjost left a comment

Choose a reason for hiding this comment

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

Looks good. Wondering if it should be more verbose. Picture a user who is picking up an fpm package from someone else. The most extreme case is probably someone who knows nothing except he has been told to do an "fpm install". Should it explicitly say something like " syntax error in fpm.toml manifest file" ? If that occurred in the package fpm.toml file you would actually get an echo of the line where the error was found with a line number, such as

--> fpm.toml:28:2-6
|
13 | [build]
| ----- test

for most errors. Also, I think putting quotes around the keyword to indicate it is a literal string would be nice. It should be a rare problem; and getting the message that it is a dependent package is a good improvement. It is easy to envision a package with dozens of dependencies where this would help identify the problem immensely.

@perazz
Copy link
Contributor Author

perazz commented Apr 27, 2023

I agree. Here I've tried to improve the clarity of the output message a bit.

Copy link
Contributor

@urbanjost urbanjost left a comment

Choose a reason for hiding this comment

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

Yes, I think that is considerably more specific without being excessively verbose. Ideally we could try messages out on novices to see if they are clear even without familiarity with fpm; it is harder to judge when you have been using it for a while but I like it.

@perazz
Copy link
Contributor Author

perazz commented Apr 28, 2023

Thank you @urbanjost, I think this is good enough for now. I will merge, let's see if the users like it better than the previous form.

@perazz perazz merged commit fc4ac0e into fortran-lang:main Apr 28, 2023
9 checks passed
@perazz perazz deleted the output_package_name_on_error branch April 28, 2023 06:22
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

3 participants