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

Create Files trait #164

Merged
merged 1 commit into from
Feb 25, 2020
Merged

Create Files trait #164

merged 1 commit into from
Feb 25, 2020

Conversation

brendanzab
Copy link
Owner

This will allow clients of codespan-reporting to implement their own file databases. This is important if we want to allow users to work with frameworks like Salsa, or have domain-specific concerns with regard to file handling.

Closes #79.

@brendanzab
Copy link
Owner Author

I'm not sure if this API would support people who wanted custom line-numbers - see #157.

Copy link
Collaborator

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This would not support custom line numbers.

codespan-reporting/src/diagnostic.rs Outdated Show resolved Hide resolved
codespan-reporting/src/diagnostic.rs Show resolved Hide resolved
column: column_index,
})
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would need to have a number() method to support #157.

"{origin}:{line}:{column}",
origin = self.origin,
line = self.location.line + 1,
column = self.location.column + 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would need to use the proposed number() API to support #157.

@brendanzab brendanzab marked this pull request as ready for review February 25, 2020 00:22
@brendanzab
Copy link
Owner Author

So yeah, this doesn't actually fix #157, but I think I'll merge it nonetheless, as it's still helpful!

@brendanzab brendanzab merged commit 9e07927 into master Feb 25, 2020
@brendanzab brendanzab deleted the files-trait branch February 25, 2020 00:23
@jyn514
Copy link
Collaborator

jyn514 commented Feb 25, 2020

So yeah, this doesn't actually fix #157, but I think I'll merge it nonetheless, as it's still helpful!

My concern wasn't that it doesn't fix #157, but rather that it doesn't allow fixing #157, because there's no way to ask the Line what line it belongs to (regardless of whether that's index + 1 or something more complicated). I'm concerned that this will make it harder to add the functionality in the future.

@brendanzab
Copy link
Owner Author

Ahh, sorry, wouldn't have merged if you were that worried! I'm definitely happy if we alter the Files API, or the Line struct before we publish 0.9 in a way that would support it.

@icsaszar icsaszar mentioned this pull request Feb 25, 2020
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.

Custom databases (Salsa support)
2 participants