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

Some major formatting, code-style and other fixes #13

Closed
wants to merge 11 commits into from

Conversation

jirutka
Copy link
Member

@jirutka jirutka commented Nov 21, 2014

No description provided.

@jirutka
Copy link
Member Author

jirutka commented Dec 9, 2014

Ping @mojavelinux, @jxxcarlson

@jxxcarlson
Copy link
Contributor

(1) The patches look good to me — but I am new at this game so don’t
know exactly what to do at this point. Please take a look at
https://github.com/jxxcarlson/asciidoctor-latex https://github.com/jxxcarlson/asciidoctor-latex. I’ve done quite a
bit of work since the repo that you are looking at — there is a sitting
pull request. The best files to look at for seeing what the converter
can do now are in the directory try-out — begin with README.adoc,
which you should render with

$ asciidoctor -r $laco -a stem=latexmath README.adoc

where $laco points to converter.rb.

The comments at https://github.com/asciidoctor/asciidoctor-latex/issues https://github.com/asciidoctor/asciidoctor-latex/issues,
issue #11, especially at the end say a few things about the current state.

(2) I notice that DocTest has progressed a lot since the last time I looked
at it. I will study it now to see how to make tests for the LaTeX back end.

On Dec 9, 2014, at 4:42 PM, Jakub Jirutka notifications@github.com wrote:

Ping @mojavelinux https://github.com/mojavelinux, @jxxcarlson https://github.com/jxxcarlson

Reply to this email directly or view it on GitHub #13 (comment).

@jxxcarlson
Copy link
Contributor

@jirutka, I started to set things up for dockets in my fork of asciidoctor-latex,
but ran into some problems — see error message below. I’ve pushed
my changes to my fork — if you have an idea of what I’m doing wrong,
that would b great.

$ be rake test
rake aborted!
NameError: undefined local variable or method `path' for #<Asciidoctor::DocTest::AsciidocRenderer:0x007fbf0aaeece8>
/Users/carlson/Dropbox/prog/git/asciidoctor-latex/Rakefile:95:in `new'
/Users/carlson/Dropbox/prog/git/asciidoctor-latex/Rakefile:95:in `<top (required)>'
(See full trace by running task with --trace)

On Dec 9, 2014, at 4:42 PM, Jakub Jirutka notifications@github.com wrote:

Ping @mojavelinux, @jxxcarlson


Reply to this email directly or view it on GitHub.

@jirutka
Copy link
Member Author

jirutka commented Dec 10, 2014

I’m at work now, I’m gonna respond to the rest of your comments in the evening.

btw Direct Messages (DM) on Twitter are super stupid, I doesn’t allow me to respond to your message when you’re not following me. :(

@jxxcarlson
Copy link
Contributor

Thanks! I will eliminate that part now & concentrate on the TeX output.

There is, however, an option where the html backend comes in to play — there are constructs
such as

[env.equation#assoc]
--
a + (b + c)  = (a + b) + c
--

where converter.rb comes in to play and produces “custom” html in the html
back end. See lines 92-158 of converter.rb, text beginning with

module Asciidoctor
  module LaTeX
    module Html5ConverterExtensions

I’m not adding or modifying the templates but rather the ruby code that drives them.

Is there a a way of testing the custom html output with DocTest?

http://noteshare.io

On Dec 10, 2014, at 8:49 AM, Jakub Jirutka notifications@github.com wrote:

template_dirs is for templates-based backends only (Slim, Haml, ERB, …), that’s not the case of the latex backend. Use renderer_opts backend_name: :latex instead.


Reply to this email directly or view it on GitHub jxxcarlson@7876810#commitcomment-8912146.

@jirutka
Copy link
Member Author

jirutka commented Dec 10, 2014

The patches look good to me — but I am new at this game so don’t know exactly what to do at this point.

If it’s okay for you, then you should merge it to the master branch – either via GitHub (Merge button on this page or manually using git.

Please take a look at https://github.com/jxxcarlson/asciidoctor-latex. I’ve done quite a bit of work since the repo that you are looking at — there is a sitting pull request.

Where’s the master branch now? This pull requests doesn’t look like focused on one particular feature, but huge ongoing development. It’s quite hard to merge contribution branches when it diverges so much. :(
You have a write access to this repo, haven’t you?

Well, these branches can’t be easily merged together, so what about this – I can apply my changes to jxxcarlson/asciidoctor-latex:master, send another PR and then you’ll merge our changes into asciidoctor/asciidoctor-latex:master. What do you think?

I notice that DocTest has progressed a lot since the last time I looked at it. I will study it now to see how to make tests for the LaTeX back end.

Great!

I started to set things up for dockets in my fork of asciidoctor-latex, but ran into some problems…

Please let’s discuss DocTest in a separate issue to keep issues well organized.
Just for the record, I’ve resolved some troubles with DocTest in commit messages here and it’s also implicitly included in #7.

@jxxcarlson
Copy link
Contributor

Jim

http://noteshare.io

On Dec 10, 2014, at 6:05 PM, Jakub Jirutka notifications@github.com wrote:

The patches look good to me — but I am new at this game so don’t know exactly what to do at this point.

If it’s okay for you, then you should merge it to the master branch – either via GitHub (Merge button on this page #13 or manually using git.

Please take a look at https://github.com/jxxcarlson/asciidoctor-latex https://github.com/jxxcarlson/asciidoctor-latex. I’ve done quite a bit of work since the repo that you are looking at — there is a sitting pull request.

Where’s the master branch now? This pull requests doesn’t look like focused on one particular feature, but huge ongoing development. It’s quite hard to merge contribution branches when it diverges so much. :(
You have a write access to this repo, haven’t you?

I don’t believe I have write access. I see this:

but according to GitHub’s instructions, I should see this:

Thus I don’t see a control to initiate a merge. Am I missing something, or do I need to somehow obtain write access?

Well, these branches can’t be easily merged together, so what about this – I can apply my changes to jxxcarlson/asciidoctor-latex:master https://github.com/jxxcarlson/asciidoctor-latex/tree/master, send another PR and then you’ll merge our changes into asciidoctor/asciidoctor-latex:master. What do you think?

I think your suggestion is the correct one — you should merge, and once I get write access and figure out how to merge, I can sort things out and we can then proceed with smaller incremental commits.

@jxxcarlson
Copy link
Contributor

I've merged the two pull requests from @jirutka and tested by pulling back to my local machine and running asciiidoctor on several files in try-out. All OK.

@jirutka
Copy link
Member Author

jirutka commented Dec 15, 2014

The third round will come this evening. 😺

Since we’re merging this PR in parts into jxxcarlson/asciidoctor-latex, I’m closing it now.

@jirutka jirutka closed this Dec 15, 2014
@mojavelinux
Copy link
Member

Great work @jirutka. I'm really glad to see your contributions on this repository. I'm confident with the two of you working together, this is going to be a solid converter!

As Jakub mentioned, we definitely want to keep branches (aka pull requests) focused on specific issues so it's easier to merge them in. Long running feature branches have the tendency of eventually becoming obsolete, which is not good for securing all the great work that was done in them.

@mojavelinux
Copy link
Member

We should be merging progress into asciidoctor/asciidoctor-latex. I've made you both admins on the repo. Please push changes upstream when you deem them ready.

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.

None yet

3 participants