Fix to allow syncing of arbitrary markdown #175

Merged
merged 3 commits into from Jun 8, 2014

Conversation

Projects
None yet
2 participants
@jasom

jasom commented Jun 5, 2014

The solution is as follows:

When syncing to trello we use the indent of the first line as the
indent for the entire description.

When syncing from trello we add a fixed indent of 2 to the
description. This prevents markdown syntax from being interpreted as
org-mode syntax.

Somewhat a workaround for issue #89 the markup isn't translated, but at
least it is preserved

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 5, 2014

Member

Hello,

Somewhat a workaround...

I do not know if it really is a workaround.
This solution is simple and permits a little more than actual code.
This is pretty cool.

Tests are ko. To merge this PR, I'd much prefer the build to be green though.

Cheers,

Member

ardumont commented Jun 5, 2014

Hello,

Somewhat a workaround...

I do not know if it really is a workaround.
This solution is simple and permits a little more than actual code.
This is pretty cool.

Tests are ko. To merge this PR, I'd much prefer the build to be green though.

Cheers,

@jasom

This comment has been minimized.

Show comment
Hide comment
@jasom

jasom Jun 6, 2014

Ah, I had an unsaved elisp change that I had eval'ed will update pull request later tonight.

jasom commented Jun 6, 2014

Ah, I had an unsaved elisp change that I had eval'ed will update pull request later tonight.

@jasom

This comment has been minimized.

Show comment
Hide comment
@jasom

jasom Jun 6, 2014

I'll wait for Travis to run, since I can't seem to get it to work; make test shows the following:

Cannot open load file: ert-expectations

jasom commented Jun 6, 2014

I'll wait for Travis to run, since I can't seem to get it to work; make test shows the following:

Cannot open load file: ert-expectations

@jasom

This comment has been minimized.

Show comment
Hide comment
@jasom

jasom Jun 6, 2014

I got tests working and found the issue; I'll have a fix soon.

jasom commented Jun 6, 2014

I got tests working and found the issue; I'll have a fix soon.

jasom added some commits Jun 5, 2014

Fix to allow syncing of arbitrary markdown
The solution is as follows:

When syncing *to* trello we use the indent of the first line as the
indent for the entire description.

When syncing *from* trello we add a fixed indent of 2 to the
description.  This prevents markdown syntax from being interpreted as
org-mode syntax.

Somewhat a workaround for issue #89 the markup isn't translated, but at
least it is preserved
Fix up tests
6 Tests expected unindented descriptions to stay unindented

Also added a simple test for writing a markdown bulleted-list to a
description.
@jasom

This comment has been minimized.

Show comment
Hide comment
@jasom

jasom Jun 6, 2014

Hold of on pulling this; a change I made to fix some of the failing tests broke multi-paragraph indenting.

jasom commented Jun 6, 2014

Hold of on pulling this; a change I made to fix some of the failing tests broke multi-paragraph indenting.

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 7, 2014

Member

I'll wait for Travis to run, since I can't seem to get it to work; make test shows the following:
Cannot open load file: ert-expectations

I'm using https://github.com/cask/cask.git for loading tests from the CLI.

Basically, for testing on your machine:

make install-cask test

(More details on README-dev.md)

Otherwise, if running from inside emacs, you'll need to install manually the tests dependencies (ert, ert-expectations, el-mock).

Cheers,

Member

ardumont commented Jun 7, 2014

I'll wait for Travis to run, since I can't seem to get it to work; make test shows the following:
Cannot open load file: ert-expectations

I'm using https://github.com/cask/cask.git for loading tests from the CLI.

Basically, for testing on your machine:

make install-cask test

(More details on README-dev.md)

Otherwise, if running from inside emacs, you'll need to install manually the tests dependencies (ert, ert-expectations, el-mock).

Cheers,

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 7, 2014

Hello,

I have some remarks:

  • As this is an internal function, can you rename it with -- after the /. This would render orgtrello-buffer/--check-indent!.
  • Also a docstring would be nice. I do not understand all that the code does.

For information, I've referenced conventions I tried to follow when developing:
https://github.com/org-trello/org-trello/blob/master/README-dev.md#conventions

Thanks.

Hello,

I have some remarks:

  • As this is an internal function, can you rename it with -- after the /. This would render orgtrello-buffer/--check-indent!.
  • Also a docstring would be nice. I do not understand all that the code does.

For information, I've referenced conventions I tried to follow when developing:
https://github.com/org-trello/org-trello/blob/master/README-dev.md#conventions

Thanks.

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 7, 2014

What does this mean?

What does this mean?

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 7, 2014

Cool!

Thanks

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 7, 2014

To follow what I said about the test target, here is how I test:

# tony at dagobah in ~/repo/perso/org-trello on git:jasom-master o [10:30:56]
$ make test
rm -rf dist/
rm -rf *.tar
cask clean-elc
cask exec emacs --batch \
                        -l ert \
                        -l ./launch-tests.el \
                        -f ert-run-tests-batch-and-exit
Loading /home/tony/repo/perso/org-trello/load-org-trello-tests.el (source)...
Launching tests!
Loading /home/tony/repo/perso/org-trello/load-org-trello.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-action.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-api.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-backend.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-buffer.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-cbx.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-controller.el (source)...
`flet' is an obsolete macro (as of 24.3); use either `cl-flet' or `cl-letf'.
Loading /home/tony/repo/perso/org-trello/org-trello-data.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-db.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-elnode.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-hash.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-input.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-log.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-proxy.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-query.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-server.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-setup.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-utils.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-webadmin.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello.el (source)...
org-trello loaded!
Loading tests done!
Loading /home/tony/repo/perso/org-trello/test/utilities-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-action-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-api-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-backend-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-buffer-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-cbx-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-controller-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-data-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-db-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-elnode-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-hash-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-proxy-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-query-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-server-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-utils-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-webadmin-tests.el (source)...
Running 747 tests (2014-06-07 10:43:56+0200)
   passed    1/747  erte-test-00001
   passed    2/747  erte-test-00002
   passed    3/747  erte-test-00003
   passed    4/747  erte-test-00004
   passed    5/747  erte-test-00005
   passed    6/747  erte-test-00006
   passed    7/747  erte-test-00007
   passed    8/747  erte-test-00008
   passed    9/747  erte-test-00009
   passed   10/747  erte-test-00010
   passed   11/747  erte-test-00011
   passed   12/747  erte-test-00012
   [truncated]
   passed  740/747  testing-orgtrello-data/merge-item
   passed  741/747  testing-orgtrello-data/parse-data-card
   passed  742/747  testing-orgtrello-data/parse-data-checklist
   passed  743/747  testing-orgtrello-data/parse-data-http-response
   passed  744/747  testing-orgtrello-data/parse-data-item
   passed  745/747  testing-orgtrello-data/parse-data-with-list-of-results
   passed  746/747  testing-orgtrello-db/behavior
   passed  747/747  testing-orgtrello-hash/init-map-from

Ran 747 tests, 747 results as expected (2014-06-07 10:43:57+0200)

To follow what I said about the test target, here is how I test:

# tony at dagobah in ~/repo/perso/org-trello on git:jasom-master o [10:30:56]
$ make test
rm -rf dist/
rm -rf *.tar
cask clean-elc
cask exec emacs --batch \
                        -l ert \
                        -l ./launch-tests.el \
                        -f ert-run-tests-batch-and-exit
Loading /home/tony/repo/perso/org-trello/load-org-trello-tests.el (source)...
Launching tests!
Loading /home/tony/repo/perso/org-trello/load-org-trello.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-action.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-api.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-backend.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-buffer.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-cbx.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-controller.el (source)...
`flet' is an obsolete macro (as of 24.3); use either `cl-flet' or `cl-letf'.
Loading /home/tony/repo/perso/org-trello/org-trello-data.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-db.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-elnode.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-hash.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-input.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-log.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-proxy.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-query.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-server.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-setup.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-utils.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello-webadmin.el (source)...
Loading /home/tony/repo/perso/org-trello/org-trello.el (source)...
org-trello loaded!
Loading tests done!
Loading /home/tony/repo/perso/org-trello/test/utilities-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-action-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-api-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-backend-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-buffer-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-cbx-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-controller-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-data-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-db-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-elnode-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-hash-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-proxy-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-query-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-server-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-utils-tests.el (source)...
Loading /home/tony/repo/perso/org-trello/test/org-trello-webadmin-tests.el (source)...
Running 747 tests (2014-06-07 10:43:56+0200)
   passed    1/747  erte-test-00001
   passed    2/747  erte-test-00002
   passed    3/747  erte-test-00003
   passed    4/747  erte-test-00004
   passed    5/747  erte-test-00005
   passed    6/747  erte-test-00006
   passed    7/747  erte-test-00007
   passed    8/747  erte-test-00008
   passed    9/747  erte-test-00009
   passed   10/747  erte-test-00010
   passed   11/747  erte-test-00011
   passed   12/747  erte-test-00012
   [truncated]
   passed  740/747  testing-orgtrello-data/merge-item
   passed  741/747  testing-orgtrello-data/parse-data-card
   passed  742/747  testing-orgtrello-data/parse-data-checklist
   passed  743/747  testing-orgtrello-data/parse-data-http-response
   passed  744/747  testing-orgtrello-data/parse-data-item
   passed  745/747  testing-orgtrello-data/parse-data-with-list-of-results
   passed  746/747  testing-orgtrello-db/behavior
   passed  747/747  testing-orgtrello-hash/init-map-from

Ran 747 tests, 747 results as expected (2014-06-07 10:43:57+0200)
@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 7, 2014

Member

Hello again,

There remains a little problem.

From your branch, creating a card like this one:

* TODO Joy of FUN(ctional) LANGUAGES
:PROPERTIES:
:END:
  - hello description
  - another
  - yet another

(- or * for list is working with your code as well, this is great news :).

Then, on the current card, sync to trello (C-c o c).

This renders a list badly formatted on trello (from the original org buffer point of view):
screenshot_2014-06-07_10-58-46

Thus, sync from trello (C-u C-c o c) renders this incorrect format:

* TODO Joy of FUN(ctional) LANGUAGES
:PROPERTIES:
:orgtrello-id: 5392ce53f5c2af311b13a83a
:END:
  - hello description
    - another
    - yet another

which is then ok in regards of trello (cf. screenshot).
So this means that the sync from does its job (which is cool).

But this is not the symmetric with the original description (which comes from the org buffer).
So, the problem must lie with the description extract function.

And, indeed it does.

Executing (orgtrello-buffer/extract-description-from-current-position!) from inside the org buffer in the original state (M-: RET (orgtrello-buffer/extract-description-from-current-position!) RET), this renders:

"- hello description
  - another
  - yet another"

So, in some way, when extracting, we must be aware of the indentation.
So the indentation remove step must take place for each line and done before the (apply 'concat ...) step.

Cheers,

Member

ardumont commented Jun 7, 2014

Hello again,

There remains a little problem.

From your branch, creating a card like this one:

* TODO Joy of FUN(ctional) LANGUAGES
:PROPERTIES:
:END:
  - hello description
  - another
  - yet another

(- or * for list is working with your code as well, this is great news :).

Then, on the current card, sync to trello (C-c o c).

This renders a list badly formatted on trello (from the original org buffer point of view):
screenshot_2014-06-07_10-58-46

Thus, sync from trello (C-u C-c o c) renders this incorrect format:

* TODO Joy of FUN(ctional) LANGUAGES
:PROPERTIES:
:orgtrello-id: 5392ce53f5c2af311b13a83a
:END:
  - hello description
    - another
    - yet another

which is then ok in regards of trello (cf. screenshot).
So this means that the sync from does its job (which is cool).

But this is not the symmetric with the original description (which comes from the org buffer).
So, the problem must lie with the description extract function.

And, indeed it does.

Executing (orgtrello-buffer/extract-description-from-current-position!) from inside the org buffer in the original state (M-: RET (orgtrello-buffer/extract-description-from-current-position!) RET), this renders:

"- hello description
  - another
  - yet another"

So, in some way, when extracting, we must be aware of the indentation.
So the indentation remove step must take place for each line and done before the (apply 'concat ...) step.

Cheers,

ardumont added a commit that referenced this pull request Jun 8, 2014

Merge pull request #175 from jasom/master
Fix to allow syncing of arbitrary markdown

@ardumont ardumont merged commit 33d07d6 into org-trello:master Jun 8, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

ardumont added a commit to ardumont/org-trello that referenced this pull request Jun 8, 2014

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 8, 2014

Member

(comment from this morning that I forgot to send).

I think I will iterate over what you proposed which is pretty cool.
Thanks for the sharing

I hope you enjoyed the exchanges as much as me :D

Cheers,

Member

ardumont commented Jun 8, 2014

(comment from this morning that I forgot to send).

I think I will iterate over what you proposed which is pretty cool.
Thanks for the sharing

I hope you enjoyed the exchanges as much as me :D

Cheers,

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 8, 2014

Member

I've iterated on this (this is on 0.4.7 branch)

At save time, the description of cards are indented (if already indented, this will do nothing).

So at description computation time, all indentation are removed.

Then permitting to have as you propose a more clean description on trello (and back on org).

Thanks.

Cheers,

Member

ardumont commented Jun 8, 2014

I've iterated on this (this is on 0.4.7 branch)

At save time, the description of cards are indented (if already indented, this will do nothing).

So at description computation time, all indentation are removed.

Then permitting to have as you propose a more clean description on trello (and back on org).

Thanks.

Cheers,

@ardumont ardumont referenced this pull request Jun 8, 2014

Merged

0.4.7 #176

5 of 5 tasks complete
@jasom

This comment has been minimized.

Show comment
Hide comment
@jasom

jasom Jun 8, 2014

I was busy yesterday so didn't have time to look over this (I think I'm 9 hours behind you, it's Sunday afternoon here). FYI the issue with this example on my original code:

* TODO Joy of FUN(ctional) LANGUAGES
:PROPERTIES:
:orgtrello-id: 5392ce53f5c2af311b13a83a
:END:
  - hello description
    - another
    - yet another

Is that the code in my pull request takes the indent from the first line of the org body, :PROPERTIES: included; since the line ":PROPERTIES:" and the line "- hello description" have different indents, that caused the problem. I'll look over the 0.4.7 branch when I get some time to see if you addressed that at all. (It's not insurmountable, just a lot more complicated than the simple change I made).

jasom commented Jun 8, 2014

I was busy yesterday so didn't have time to look over this (I think I'm 9 hours behind you, it's Sunday afternoon here). FYI the issue with this example on my original code:

* TODO Joy of FUN(ctional) LANGUAGES
:PROPERTIES:
:orgtrello-id: 5392ce53f5c2af311b13a83a
:END:
  - hello description
    - another
    - yet another

Is that the code in my pull request takes the indent from the first line of the org body, :PROPERTIES: included; since the line ":PROPERTIES:" and the line "- hello description" have different indents, that caused the problem. I'll look over the 0.4.7 branch when I get some time to see if you addressed that at all. (It's not insurmountable, just a lot more complicated than the simple change I made).

@ardumont

This comment has been minimized.

Show comment
Hide comment
@ardumont

ardumont Jun 10, 2014

Member

Hello,

I was busy yesterday so didn't have time to look over this (I think I'm 9 hours behind you, it's Sunday afternoon here).

No problem.
I merged and tried to improve on your improvments :D

I'll look over the 0.4.7 branch when I get some time to see if you addressed that at all. (It's not insurmountable, just a lot more complicated than the simple change I made).

I believe I did :D
But to have another pair of eyes is always better.

I changed the way the card description is extracted.
I now compute the description's starting point exactly (so I no longer have to filter out the card properties). And then I cut every description lines by the indentation set earlier. And now the description is good for being synced.

Also, to ensure that nothing is lost during description extraction, a function is installed which does force the indentation on description region for every card of the buffer at each save time.
(I also kept how you write the description with indentation - I just refactored this in a function - but i believe I could have removed it...).

Cheers,

Member

ardumont commented Jun 10, 2014

Hello,

I was busy yesterday so didn't have time to look over this (I think I'm 9 hours behind you, it's Sunday afternoon here).

No problem.
I merged and tried to improve on your improvments :D

I'll look over the 0.4.7 branch when I get some time to see if you addressed that at all. (It's not insurmountable, just a lot more complicated than the simple change I made).

I believe I did :D
But to have another pair of eyes is always better.

I changed the way the card description is extracted.
I now compute the description's starting point exactly (so I no longer have to filter out the card properties). And then I cut every description lines by the indentation set earlier. And now the description is good for being synced.

Also, to ensure that nothing is lost during description extraction, a function is installed which does force the indentation on description region for every card of the buffer at each save time.
(I also kept how you write the description with indentation - I just refactored this in a function - but i believe I could have removed it...).

Cheers,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment