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

0.5 comments #39

Merged
merged 1 commit into from
Dec 20, 2017
Merged

0.5 comments #39

merged 1 commit into from
Dec 20, 2017

Conversation

zbraniecki
Copy link
Collaborator

Built on top of the private msg PR. Need to add tests, but hopefully a good start :)

fn get_comment<I>(ps: &mut ParserStream<I>) -> Result<ast::Comment>
#[derive(PartialEq)]
enum CommentLevel {
Comment,
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use the Comment = 0, syntax in this enum to avoid the match a few lines below. I think you'll need to add Copy to the enum though to make it work.

@@ -152,6 +153,13 @@ where
{
let id = get_identifier(ps, true)?;

if comment.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this outer is_some needed here? It looks like the if let below it serves the same function (and more).

@zbraniecki zbraniecki force-pushed the 0.5-comments branch 2 times, most recently from 0f0da93 to d749049 Compare December 19, 2017 19:43
@@ -0,0 +1,721 @@
pub use super::errors::ParserError;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file.

@@ -152,6 +143,11 @@ where
{
let id = get_private_identifier(ps)?;

if let Some(ast::Comment::Comment { .. }) = comment {
} else if comment.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this code. It looks like both if let and else if test if comment is a Some?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

heh, yeah, it's awkward. What I'm trying to test is if the variant of the Comment passed to get_message is actually ast::Comment::Comment and not GroupComment or ResourceComment.

If it's not, then I'm verifying that it's None.

Any ideas how to write it better? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I see now, thanks. I might have wrongly suggested to use if let here then. Maybe a match like this?

match comment {
    Some(ast::Comment::GroupComment { .. }) => error!(),
    Some(ast::Comment::ResourceComment { .. }) => error!(),
    _ => ()
}

In the future, rust-lang/rfcs#935 will help.

@zbraniecki zbraniecki merged commit 2e7f597 into projectfluent:master Dec 20, 2017
@zbraniecki zbraniecki deleted the 0.5-comments branch December 20, 2017 09:40
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