search

Remillard / VHDL-Mode

A package for Sublime Text that aids coding in the VHDL language.
MIT License
40 stars 10 forks source link

Small syntax highlighting fixes #82

Closed Sv3n closed 6 years ago

Sv3n commented 6 years ago

This pr fixes some cases where the syntax highlighting on keywords was case sensitive, and one case where modes (in, out, inout) were highlighted within (function) parameter name declarations (e.g. my_input would highlight as my_input).

@Remillard (I saw you previous comment but rolled back my fork to fix the email address under which the commits were made. I'm happy to see development on a proper new-style syntax file for VHDL for Sublime).

Remillard commented 6 years ago

You're the first person outside myself to submit code so there's been no reason to organize this before now. Typically I commit all code to develop, and then I roll code into master for releases. Could you withdraw the pull request and resubmit to target develop? This is kind of minor but it keeps the branching more organized.

Also I'm wondering on the modes if it would be better to look for \b({{modes}})\b instead of the spaces. The syntax engine consumes things when it matches so one these bits where a space is part of the regexp, it's less intrusive to look for word boundaries (in case a later match needs to consume a leading space). And again, if it's in develop, and since my local repository is synced with develop, then I can pull that forward and test the changes out a little bit before committing to master.

Remillard commented 6 years ago

HEY... cool beans, I found how to retarget the pull request. So, you don't have to do a thing. Could you check the one change there about using the non-consuming \b and let me know. Then I'll merge it. I already have some release documentation updated for your changes that I'll submit and then can release it.

Remillard commented 6 years ago

Okay, that did not do exactly what I thought it would do. I have no idea why this did that. Frustrating.

Remillard commented 6 years ago

Okay. master and develop were synchronized, however you forked from master, so if we PR to develop, I think it's just picking up all the master PRs up to this point. The PR still says only one file is changed, so I thikn it's okay even if it looks very noisy.

Remillard commented 6 years ago

I gave it some thought and I think the space is probably fine. I'm going to approve and merge.

Sv3n commented 6 years ago

Great! I didn't follow this thread while it was happening, but the end result looks as intended :)

Remillard commented 6 years ago

Yeah I chatted with some of the other ST devs in Discord and it's just my inexperience with Github.

For my purposes, I have two branches, master and develop. I have copies of these locally, however I usually just push from local-develop to GitHub-develop. Then when I am ready to release I do a single PR from GitHub-develop to GitHub-master, and tag the release. Then I fast forward pull local-master and everything is caught up.

What happened is that you forked from GitHub-master and created a pull request to GitHub-master. I changed your PR to target GitHub-develop. Well... the FILES were all the same, but GitHub-develop has never seen the pull request tags since I always pushed! For the first time, develop saw stuff coming the other way and so the newly edited pull request was tagged with every PR title since near the beginning of the development.

So... mainly it was just really MESSY, but yes, the end result was exactly the same. The PR showed that only one file was altered, and that's what we wanted.

I made a CONTRIBUTING file per GitHub's suggestion and also put some notes in the documentation in case folks want to contribute further in the future. Ideally folks will fork GitHub-develop, and then PR back to GitHub-develop. I think then everything will go in the right direction. At least now I know what I'm seeing when that happens in the future too.

Anyhow, you should also see that documentation indicates you as a contributor (useful in case you ever use open-source work as CV material) and I released the fix pretty quickly on Friday.

Thanks for taking a look at this!

Best regards, Mark

On Mon, Jan 8, 2018 at 10:41 AM Sven Goossens notifications@github.com wrote:

Great! I didn't follow this thread while it was happening, but the end result looks as intended :)

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/Remillard/VHDL-Mode/pull/82#issuecomment-356020925, or mute the thread https://github.com/notifications/unsubscribe-auth/AatvGVWAzqYvuyAgTY_6t0U5_82z86YFks5tIkU3gaJpZM4RUMJo .