-
Notifications
You must be signed in to change notification settings - Fork 414
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
Fix abstract injection #601
Conversation
Thanks for doing this, @thibaudrobelain. This looks good to me, though I'm not 100% sold on mixing the auto-generated abstracts with custom ones. For example, appending "The following enums are available globally." to the custom abstracts. Since @agentk and @pcantrell are some of the biggest users of this, what do you think? PS: I've merged realm/jazzy-integration-specs#21 into master so you can point this branch back to the realm repo. |
No strong opinion here; I haven’t used My gut feeling is that text like “The following enums are available globally” is placeholder and a custom abstract should replace it, whereas text that comes from That may be overcomplicating things, though, and if I had to choose, I’d preserve doc comments even if it also meant sometimes preserving boilerplate. |
We only ever use it for custom sections, where it's not possible to put the content in comments in the code. So it's never a clash for me either. To @pcantrell's point, I don't know if it's possible to differentiate between placeholder text and docs generated from comments. But if it replaced placeholder text and prepended code comment content, I would be all for it. I think from that way at least it is making it obvious as to what is happening and that the user could just move the abstract back into the class at that point. |
7159fbd
to
ab669f7
Compare
Rebased with master and pointed it towards realm/jazzy-integration-specs again. However integration specs are failing because @pcantrell's stuff isn't on master yet! I've briefly looked at trying to differentiate between placeholder text and code comments and it seems like it'd maybe make things a bit messier to try and differentiate between the two. It seems like you don't have a strong opinion on this, so I'd be fine with starting off with the current behavior. When I find some time I could always give a go to the "upgraded" functionality. Let me know what you think! |
Hey @jpsim, any new thoughts on this? |
I'm still in favour of this change for the purpose of not behaving in a 'magic' way and removing abstracts if a file of the same name exists. As soon as the integration tests are sorted. |
ab669f7
to
ff9bb85
Compare
I'd like to attempt getting this merged in again so I rebased on the current master, however based on what I've seen locally it seems like the integration specs are not fully up to date? |
Okay; after a quick investigation, since I've switched to Xcode 8 it seems like the whole output is messed up because it expects the "Use legacy swift version" of each module to be set now. Using xcode-select with Xcode 7.3.1 seems to fix part of it but then it complains some more about CoreSimulator. Didn't investigate much more on this because the travis output seems like it's the correct one since it's still using Xcode 7.3.1: I could simply take the individual file diffs from there and commit them in the integration-specs and travis would be happy with this branch. What do you think? The diff is basically the same that was merged at some point and reverted since this branch never made it onto master. |
aba3659
to
20029fb
Compare
Waiting on realm/jazzy-integration-specs#22. Then we can merge this. |
20029fb
to
8cd85f1
Compare
Thanks for your continued work on this @thibaudrobelain and sorry for the delay. I'm working on getting this in now. |
Continued in #695. Many thanks @thibaudrobelain. |
This fixes the behavior of --abstract as described in #600.
Integration specs currently point at my fork but I'll change that if we approve the new behavior!
This PR fixes the --abstract command when it's being used with existing sections that have an already existing abstract. The old behavior was to remove it and use the alternative abstract given from the .md file entirely, the new behavior (this PR) is to simply append the existing abstract after the alternative abstract.