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

Fix - Ensure there is a newline after each description. org-trello/org-... #173

Merged
merged 2 commits into from
May 30, 2014

Conversation

dtebbs
Copy link
Contributor

@dtebbs dtebbs commented May 29, 2014

...trello#172

I don't know enough about to emacs-lisp to make the proper fix. This change just blindly adds a newline after each Description, so can result in extra lines between cards.

@dtebbs
Copy link
Contributor Author

dtebbs commented May 29, 2014

My pull-request can't really be used as-is. As I sync back and forth between the buffer and the board, I think it's adding more newlines.

The correct fix would be appreciated.

@dtebbs
Copy link
Contributor Author

dtebbs commented May 29, 2014

I added a test to show this problem up and pushed a new change.
You may know of other conditions under which a newline doesn't get written.

@dtebbs
Copy link
Contributor Author

dtebbs commented May 29, 2014

Btw, I still think write-header! should always handle inserting the newline, since that seems to be the behavior expected from other functions, but that would involve changes to the code that selects the header region, which would take me a very long time given my lack of any elisp skills.

@ardumont
Copy link
Member

Hello,

My pull-request can't really be used as-is.
As I sync back and forth between the buffer and the board, I think it's adding more newlines.

It is possible indeed that the change unfortunately introduced some inconsistent behavior depending on the usage.

The correct fix would be appreciated.

+1
I will see what I can do.

I added a test to show this problem up and pushed a new change.

+1

You may know of other conditions under which a newline doesn't get written.

Right now, I do not remember.
I need to investigate.

Thanks for the issue and the fix tryout.

Cheers,

@dtebbs
Copy link
Contributor Author

dtebbs commented May 29, 2014

Just to be clear, with this second commit all tests now pass (including the new one for the original issue). The Travis CI build appears to be failing for other reasons:

Cask could not be bootstrapped. Try again later, or report an issue at https://github.com/cask/cask/issues

are you happy to accept this patch or are you going to investigate yourself first?
Just wondering.

@ardumont
Copy link
Member

Hello,

Btw, I still think write-header! should always handle inserting the newline, since that seems to be the behavior expected from other functions, but that would involve changes to the code that selects the header region, which would take me a very long time given my lack of any elisp skills.

I did not see this message when I replied.
Did you mean write-card-header!?

Just to be clear, with this second commit all tests now pass (including the new one for the original issue).

Yes.

The Travis CI build appears to be failing for other reasons:

Yes, the CI has some dependencies which is broken independent from the tests.
I checked out your branch and confirm that all tests are ok..

Are you happy to accept this patch or are you going to investigate yourself first?

I'm happy with it but I'd like to ensure there are no side-effects.

So I'd like to investigate
But, right now, I have another problem independent from this issue with my own org-trello install.
(There is brick inside org-trello which no longer does its job. I did not change the code so this may be install related)

That's why it takes me time right now.

I have questions that I'm used to answer before merging.
Have you tried the different merge-from behavior on the different entities?

Just wondering.

I know the feeling :D

Cheers,

@dtebbs
Copy link
Contributor Author

dtebbs commented May 30, 2014

Thanks for your answer. I didn't mean to push, just want to make sure the state of the second commit was noticed.

Did you mean write-card-header!?

yes, write-card-header! is what I meant :)

Have you tried the different merge-from behavior on the different entities?

Sorry, I don't understand. Is it something that is not covered by the tests?

Thanks again. Good luck with the install issue.

@ardumont
Copy link
Member

I didn't mean to push, just want to make sure the state of the second commit was noticed.

No worries about the pushing.
I did not take it that way :D

Sorry, I don't understand. Is it something that is not covered by the tests?

org-trello is not bullet-proof yet.

It's just an old habit of mine to test different things including (especially in this case because it's what was touched) :

  • sync from trello the entire buffer (C-u C-c o s)
  • sync card/checklist/item from trello without structure (C-u C-c o c)
  • sync card/checklist/item from trello with structure (C-u C-c o C)

Here for example, to ascertain that the text does not move (that is new line inserted at each turn).

Thanks again.

No problem

Cheers,

@ardumont
Copy link
Member

Good luck with the install issue.

Thanks.
It's not your concern but I'm dancing right now and I like to share some joy.
I've found the problem.
monkey-patching is evil...
prelude redefines a function json-encode-hash-table used by org-trello and this redefinition breaks it...
The tests were ok anyway because they run with cask (without prelude)...
(There is something wrong in my dev stack...)

Anyway, this means I will be able to concentrate on this problem... as soon as I found out how to avoid this.

Cheers,

@ardumont
Copy link
Member

Ok.

I checkout-ed the branch again and tested to see what I wanted to see.
I see no blocking point.
Cool.

Thanks for this fix.
It's greatly appreciated!

By merging this, I will let melpa do its job.
I will make another stable release later including this of course.

Cheers,

ardumont added a commit that referenced this pull request May 30, 2014
Fix - Ensure there is a newline after each description.  org-trello/org-...
@ardumont ardumont merged commit 1b46b9d into org-trello:master May 30, 2014
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