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

Added nesasm lexer #1354

Merged
merged 13 commits into from
Nov 12, 2019
Merged

Added nesasm lexer #1354

merged 13 commits into from
Nov 12, 2019

Conversation

ShyDev
Copy link
Contributor

@ShyDev ShyDev commented Nov 3, 2019

6502 assembler geared towards NES
http://nespowerpak.com/nesasm/

Never even saw ruby code before, never done pull requests.
Hopefully I did not screw up completely

@pyrmont pyrmont self-assigned this Nov 3, 2019
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Nov 3, 2019
Copy link
Contributor

@pyrmont pyrmont left a comment

Choose a reason for hiding this comment

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

Thanks for the the PR! I've made some changes and tried to explain them here with comments. Please let me know if I've inadvertently broken anything or if anything doesn't make sense!

lib/rouge/lexers/nesasm.rb Show resolved Hide resolved
lib/rouge/lexers/nesasm.rb Show resolved Hide resolved
lib/rouge/lexers/nesasm.rb Show resolved Hide resolved
lib/rouge/lexers/nesasm.rb Outdated Show resolved Hide resolved
@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Nov 11, 2019
@ShyDev
Copy link
Contributor Author

ShyDev commented Nov 12, 2019

Thanks for your time cleaning up and improving my code. It's all working great.
Can I add 2 simple rules in this pull request or it's better to open new pull request for this?
And one more question. For builtin function and macro parameters what token should I use? Name::Builtin is ok?

@pyrmont
Copy link
Contributor

pyrmont commented Nov 12, 2019

@ShyDev Oh, please push them to your fork. The PR is synced with it so anything you push there will show up here. I've got commit rights to that fork so the changes I've made here have actually been to your fork; make sure you pull them down with git pull if you haven't done so already and you'll be good to go.

@pyrmont
Copy link
Contributor

pyrmont commented Nov 12, 2019

As for the right token, it's usually more of an art than a science. Name::Builtin sounds like a good choice to me.

lib/rouge/lexers/nesasm.rb Show resolved Hide resolved
@pyrmont pyrmont merged commit 374ee22 into rouge-ruby:master Nov 12, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Nov 12, 2019

@ShyDev Thanks for submitting the lexer—it all looks good to me! This will be part of v3.14.0 when it comes out on 10 December 🎉

@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Nov 12, 2019
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.

2 participants