8000 Allowing YARD to parse content of C++ namespaces. Fixes #809. by thomthom · Pull Request #1063 · lsegal/yard · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allowing YARD to parse content of C++ namespaces. Fixes #809. #1063

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 20, 2017
Merged

Allowing YARD to parse content of C++ namespaces. Fixes #809. #1063

merged 1 commit into from
Jan 20, 2017

Conversation

thomthom
Copy link
Contributor
@thomthom thomthom commented Jan 19, 2017 •
edited
Loading

Description

This Pull Request attempts to address #809 - where content inside of C++ namespaces isn't processed. This is an issue for source code that use the Ruby C API in a C++ environment.

What this fix do is simply make the C Parser not treat the content of namespaces as body statements. Instead it just ignores the namespaces all together.

I'm not sure if it's perfect, but we're working on getting a very sizable amount of code parsed so I'll follow up with improvements as needed if we run into them.

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@coveralls
Copy link
coveralls commented Jan 19, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 93.513% when pulling 7a76685 on thomthom:dev-cpp-namespace into 7217607 on lsegal:master.

@thomthom
Copy link
Contributor Author

I had to add another commit to correct a last moment issue that made RuboCop fail. Do I have to go back and squash this? Or can you just use the GitHub Merge and Squash feature?

I was at first not running rubocop before I submitted because I threw errors on all files due to line endings. That's because git normalize to OS line endings upon checkout - but then correct them when you check in.

I ended up adding these lines temporarily to .rubocop to mute those warnings:

EndOfLine:
  Enabled: false

Since git normalize line ending upon checking, is that RuboCop check really needed?

@thomthom
Copy link
Contributor Author

hmm... hold this PR. I might have run into a regression. Let me run more tests to be sure.

thomthom
thomthom commented Jan 19, 2017
View reviewed changes
Gemfile
@@ -2,7 +2,7 @@
source 'https://rubygems.org'

group :development do
gem 'rspec', '~> 3.0'
gem 'rspec', '~> 3.5'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so I had not intended this to be part of this PR. But this was something I had to do in order to get things running. See #1062

Copy link
Owner

Choose a reason for hiding this comment

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

Undo this change and rebase off of master since this fix was pushed separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. So when rebasing, do I then create a new PR? since that changes the history of the branch. (I'm not that familiar with rebase.)

@coveralls
Copy link
coveralls commented Jan 19, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 93.515% when pulling b83080f on thomthom:dev-cpp-namespace into 7217607 on lsegal:master.

thomthom
thomthom commented Jan 19, 2017
View reviewed changes
lib/yard/parser/c/c_parser.rb
@@ -38,6 +38,7 @@ def parse_toplevel
when '#'; consume_directive
when '/'; consume_comment
when /\s/; consume_whitespace
when '}'; advance # Skip possible C++ namespace closing brackets.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this one, I'm not sure if you'll like this much. I can try to refactor it if you prefer. Though any suggestions would be appreciated. The issue here was that when there are namespaces my fix was to just ignore it. But then consume_toplevel_statement would later run into stray } (closing namespaces) that would throw it off.

So this line is currently very generic in advancing over any } in the top level. I could try to keep track of the depth of the C++ namespaces and only advance over } a matching number of times.

It all depend on how low impact we want to keep this. I ran this fix over our code base with over 1300 methods and 98 classes - no documentation objects where lost.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not too concerned, since the worst case is that it continues past a toplevel element to find the next one.

@coveralls
Copy link
coveralls commented Jan 20, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 93.515% when pulling 405f0b1 on thomthom:dev-cpp-namespace into d527052 on lsegal:master.

@lsegal lsegal merged commit 63d546c into lsegal:master Jan 20, 2017
@thomthom thomthom deleted the dev-cpp-namespace branch January 20, 2017 01:13
lsegal added a commit that referenced this pull request Apr 23, 2017
@lsegal
Tag release v0.9.9
7c5682b 
References:
  #1057, #1062, #1063, #1067, #1069, #1075, #1078, #1082,
  #1083, #809
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsegal lsegal lsegal left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thomthom @coveralls @lsegal
0