Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Added impl capture. #96

Merged
merged 6 commits into from
May 2, 2017
Merged

Added impl capture. #96

merged 6 commits into from
May 2, 2017

Conversation

maccoda
Copy link
Contributor

@maccoda maccoda commented Feb 17, 2017

Hopefully able to fix issues on incorrect impl highlighting like #7 and maybe #86

@@ -404,14 +404,16 @@
]
}
# Implementation
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose you shouldn't have removed the line.
Why did you do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was accidental! 😱 Apologies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@maccoda,
Okay. Revert it and the brace at the end which you have deleted.

@KalitaAlexey
Copy link
Collaborator

Hello @maccoda,
Thank you for your PR.
Currently I am short of time hence I am not able to check it myself.
Could you attach screenshots?
For the following impls:

impl A {}
impl A for B {}
impl<A> B for C {}
impl<A> B<A> for C<A> {}
impl<'a> B for C {}
impl<'a> B<'a> for C<'a> {}
impl<'a, A> B<'a> for C<'a> {}
impl<'a, A> B<'a, A> for C<'a, A> {}

@maccoda
Copy link
Contributor Author

maccoda commented Feb 17, 2017

Hi,
You are welcome. No worries will use those as tests. The last three examples do not appear to work off the regex so I will work on those and then update.

@@ -429,5 +432,5 @@
'match': '\\bfor\\b'
}
]
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

You forgot about the brace.

@KalitaAlexey
Copy link
Collaborator

The last three examples do not appear to work off the regex so I will work on those and then update.

I hope you find the regex.
If you didn't I would accept the PR anyway because it is improvement.

@maccoda
Copy link
Contributor Author

maccoda commented Feb 17, 2017

Alright so it turns out those missing braces are what helped actually because I believe there is an order in which Atom checks and because this is a complex one grabbing the entire line it should be up the top. Hence the move you will see in the next commit. I have also attached a few screenshots of the tests you provided.

@maccoda
Copy link
Contributor Author

maccoda commented Feb 17, 2017

As you will be able to see the trait and type/struct names are now highlighted but next goal will be to get the type parameters also captured.
atom_dark
newton_dark
one_dark
one_light

@KalitaAlexey
Copy link
Collaborator

KalitaAlexey commented Feb 17, 2017

Please attach a screenshot for the code:

'a;
&'a i; 

I want 'a in impl to be drawn like in the example above.

@KalitaAlexey
Copy link
Collaborator

Also I want A in <A> to be drawn as A in impl A.

@maccoda
Copy link
Contributor Author

maccoda commented Feb 17, 2017

OK I will have a look into that and see what I can get

@maccoda
Copy link
Contributor Author

maccoda commented Feb 17, 2017

Newton Dark
newton_dark
One Dark
one_dark
One Light
one_light

Copy link
Collaborator

@KalitaAlexey KalitaAlexey left a comment

Choose a reason for hiding this comment

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

It is cool.

@@ -159,11 +159,46 @@
{ 'include': '#std_types' }
{ 'include': '#std_traits' }
{ 'include': '#type_params' }
{
'name': 'entity.name.type.rust'
'match': '\\b[A-Za-z]+\\b'
Copy link
Collaborator

Choose a reason for hiding this comment

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

A type's name may contain digits and underscore:
https://is.gd/5NukLU

{
'name': 'entity.name.type.rust'
'match': '\\b[A-Za-z]+\\b'
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extract the block from here and place it after 144 line:

   'std_traits': {
     'comment': 'Standard library trait'
     'name': 'support.type.std.rust'
     'match': '\\b(ToOwned|ToString)\\b'
   }
   'Type': {
     'comment': 'A type'
     'name': 'entity.name.type.rust'
     'match': '\\b[A-Za-z]+\\b'
   }

And add after 161 line:

{ 'include': '#type' }

}



Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the empty lines.

{
'name': 'entity.name.type.rust'
'match': '\\b[A-Za-z]+\\b'
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the "#Type" from the previous comment.

@maccoda
Copy link
Contributor Author

maccoda commented Feb 18, 2017

Addressed your comments. Hope this is closer to what we are wanting.

@KalitaAlexey
Copy link
Collaborator

@maccoda,
I am going to check it.
After that I will merge it.

Thank you

@KalitaAlexey
Copy link
Collaborator

@maccoda,
I have checked it.
Everything is fine.

@KalitaAlexey
Copy link
Collaborator

KalitaAlexey commented Feb 19, 2017

@maccoda,
Except that tests are failed.

@KalitaAlexey
Copy link
Collaborator

KalitaAlexey commented Feb 19, 2017

@maccoda,
Could you fix the tests and add new ones for the cases?

'type': {
'comment': 'A type'
'name': 'entity.name.type.rust'
'match': '\\b[A-Za-z0-9_]+\\b'
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is wrong regex because it matches for _, 0 and etc.

The correct is \\b([A-Za-z][A-Za-z0-9_]*|_[A-Za-z][A-Za-z0-9_]*)\\b.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The correct is \\b([A-Za-z][_A-Za-z0-9]*|_[_A-Za-z0-9]+)\\b

# Implementation
{
'comment': 'Implementation'
'begin': '\\b(impl)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be \\b(impl)\\b because otherwise it would match implementation.

@maccoda
Copy link
Contributor Author

maccoda commented Apr 25, 2017

Sorry took so long to get back to this, I believe the previous regex didn't work correctly for something like

fn do_whatever<T: Send+Share+Whatever, U: Freeze> (param: &T, other_param: u32) -> Option<U>;

I believe now all the tests should be passing and your comments addressed.
newton_dark
one_dark
one_light

@KalitaAlexey
Copy link
Collaborator

@maccoda,
Thank you for your great work.
As soon as I check it, I will merge it.

@KalitaAlexey
Copy link
Collaborator

Sorry I forgot about you. I will look tomorrow.

@KalitaAlexey
Copy link
Collaborator

Oups. I did it again. I will look at it now.

@KalitaAlexey
Copy link
Collaborator

I have prepared tests for your changes.
Unfortunately, not everything works as I want it to work.
It works incorrectly for:

impl<'a, T> A {}

It tokenizes , T as a single type parameter.
I am going to merge your changes as is because it is hard work.
I will fix the remaining cases myself.

@KalitaAlexey KalitaAlexey merged commit c24ef01 into zargony:master May 2, 2017
@KalitaAlexey
Copy link
Collaborator

@aeschli,
Hello. I am writing to you since you told me to notify you when I want to update update the grammar.
Could you please update the grammar?

@maccoda
Copy link
Contributor Author

maccoda commented May 2, 2017

@KalitaAlexey thanks for that, sorry didn't get as you were wanting but hopefully it is a step in the right direction ☺️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants