New Feature: Import jrnl files #51

aniketpant · 2013-08-12T08:47:09Z

New feature added for importing jrnl files.

$ jekyll import jrnl

Available overrides

--file: Path to input file (default: ~/journal.txt)
--time_format: the time format used by jrnl (default: %Y-%m-%d %H:%M)
--extension: file extension of the output file (default: md)
--layout: set layout of the output file (default: post)

Usage

# will use ~/path/to/file/work.txt as the jrnl file
$ jekyll import jrnl --file ~/path/to/file/work.txt

# will output all files as txt files
# this feature is yet to be implemented due to contraints from jekyll
$ jekyll import jrnl --extension txt

# will output all files with YAML data having layout: custom
# this feature is yet to be implemented due to contraints from jekyll
$ jekyll import jrnl --layout custom

Signed-off-by: Aniket Pant <me@aniketpant.com>

aniketpant · 2013-08-12T09:07:08Z

@parkr I didn't realize that overrides wouldn't work straight away. How do I allow the usage of --time_format, --extension and --layout?

Signed-off-by: Aniket Pant <me@aniketpant.com>

parkr · 2013-08-12T15:04:50Z

lib/jekyll/jekyll-import/jrnl.rb

+      extension = options[:extension] || "md"
+      layout = options[:layout] || "post"
+
+      date_length = Time.now().strftime(time_format).length


Unlike Python, you don't need the parentheses here for Time.now(). You can just call Time.now.strftime #... :)

@parkr Is the change required due to coding guidelines?

Yes :)

Omit the parentheses when the method doesn't accept any arguments.

– GitHub Ruby Styleguide

I'll add it to the CONTRIBUTING file if it isn't already there.

parkr · 2013-08-12T15:17:43Z

Nice work! Looks great so far. One suggestion I would make is to encapsulate various functionality into methods and call them from the main method. :)

aniketpant · 2013-08-12T15:21:24Z

@parkr I am not clear with what you mean by that or maybe I didn't understand. Can you elaborate on which functionalities you want to be called form the main method?

Also, I had asked this question before, you might have missed it:

@parkr I didn't realize that overrides wouldn't work straight away. How do I allow the usage of --time_format, --extension and --layout?

Thanks :)

parkr · 2013-08-12T16:10:07Z

I just mean that the various logic it'd be cool if you split out into separate methods so the whole process wasn't one big method with all the nuts-and-bolts code packed into it.

At the moment, all options have to be added to the main Jekyll gem in bin/jekyll.

aniketpant · 2013-08-12T16:26:31Z

I just mean that the various logic it'd be cool if you split out into separate methods so the whole process wasn't one big method with all the nuts-and-bolts code packed into it.

Ah. I was planning to do that at the start but then I saw that the other migrators were also written as a single block of code. It's always good to decouple code.

At the moment, all options have to be added to the main Jekyll gem in bin/jekyll.

I noticed that there is not provision for the addition of new overrides via CLI because there are only six of them available and they are hard coded. This gives me the idea to improve that code and allow the use of new overrides. Will try working on something. For now I will let the configuration I have provided in lines 17-19 (defaults are meaningful, they won't disturb anything). Once new overrides can be created, users will have more control over the imports.

parkr · 2013-08-12T16:30:05Z

Sorry for not being clear enough before! Thanks for being patient with me. :)

This gives me the idea to improve that code and allow the use of new overrides. Will try working on something.

Check out the commando branch on both repos - @mojombo was working on something similar a couple months a go - would you mind starting there? It allows each migrator to specify its own options, which is probably the solution we most want here. What do you think?

aniketpant · 2013-08-12T16:34:30Z

commando looks brute. I am going to start digging in the code. Can you update me on the work that has been going on it? (maybe mail or if any discussion is already going on the same)

parkr · 2013-08-12T16:59:37Z

I unfortunately don't know much about what Tom was working on on that branch. I looked through it briefly once but wasn't sure where I needed to go from there. I'll rebase and look through it now :)

Signed-off-by: Aniket Pant <me@aniketpant.com>

aniketpant · 2013-09-23T17:02:43Z

@parkr About time we merged this?

parkr · 2013-09-23T19:47:44Z

jekyll-import.gemspec

@@ -6,7 +6,7 @@ Gem::Specification.new do |s|

  s.name              = 'jekyll-import'
  s.version           = '0.1.0.beta3'
-  s.date              = '2013-07-14'
+  s.date              = '2013-08-12'


Would you mind reverting this change?

parkr · 2013-09-23T19:50:01Z

Yes, I agree! Thanks for pinging.

Signed-off-by: Aniket Pant <me@aniketpant.com>

aniketpant · 2013-09-24T00:51:16Z

@parkr Done.

parkr · 2013-09-24T01:03:30Z

lib/jekyll/jekyll-import/jrnl.rb

+      # convert relative to absolute if needed
+      file = File.expand_path(file)
+
+      abort "The jrnl file was not found. Please make sure '#{file}' exists. You can specify a different file using the --file switch." unless File.file?(file)


Just thinking about this again: does the --file switch actually work in your tests?

The --file switch works. I am using ~/journal.txt as the default because that's a jrnl default too. And then on L26, I check for the availability of the file.

Awesome! I just wanted to make sure --file works.

parkr · 2013-09-24T02:09:32Z

One more comment above then we should be good to go.

Want to try your hand at tests, or want us to do that?

aniketpant · 2013-09-24T02:14:57Z

Me me me! I want to try writing tests. Just give me brief intro. I have
some testing before but never in Ruby.

parkr · 2013-09-24T02:29:30Z

Take a look at the other files in test/. You're going to want to add a test/test_jrnl_importer.rb that takes "fake" data and ensures that the output is correct. Try to just make sure you get the logic right (may need to refactor the class into more methods for easier testing) so that when we make changes, we don't muck anything up in the future. Oh and please try to avoid I/O (so no writing of the files).

parkr · 2013-09-24T02:29:36Z

:D

aniketpant · 2013-09-24T02:31:53Z

Got it. Will write tests and let you know. If I have any doubts/questions,
will ask you on Twitter.

parkr · 2013-09-24T02:33:11Z

Sounds good! Or email me parker AT parkermoo.re

Signed-off-by: Aniket Pant <me@aniketpant.com>

aniketpant · 2013-09-25T08:47:35Z

@parkr Wrote the tests. Take a look at them and see if they do the job.

P.S. I wish we had mocha and chai for Ruby.

mattr- · 2013-09-25T13:31:19Z

lib/jekyll/jekyll-import/jrnl.rb

+    # Returns array converted to YAML
+    def self.create_meta(layout, title, date)
+      data = {
+        'layout'        => layout.to_s,


The calls to to_s aren't needed here or on the line below. You're already working with Strings.

mattr- · 2013-09-25T13:31:46Z

Looks good to me, other than one small nit.

Signed-off-by: Aniket Pant <me@aniketpant.com>

parkr · 2013-09-25T17:26:07Z

Looks like the tests aren't passing on Travis. Did you commit all your files and push up the latest?

aniketpant · 2013-09-25T17:43:41Z

Yeah. I did. Could you please look into what's wrong?

I know that I should've removed that date test but that can't be the
reason as it passed OK on local.

parkr · 2013-09-26T02:50:32Z

test/test_jrnl_importer.rb

+    end
+
+    should "have date" do
+      assert_equal("2013-09-24 11:36:00 +0530", "#{JekyllImport::Jrnl.get_date(@entry[0], @date_length)}")


You'll need to build a Time object instead of just comparing to this String. :)

Why so?

The method returns a string on it's own.

Time#parse returns a Time object: https://github.com/jekyll/jekyll-import/pull/51/files#L2R53

Signed-off-by: Aniket Pant <me@aniketpant.com>

aniketpant · 2013-09-26T16:27:27Z

@parkr The test is still failing. I am not sure what is the problem because I have changed the test now.

parkr · 2013-09-26T16:47:39Z

Ha! I had a feeling that was going to come back to bite us in the butt.

[ 2/31] TestJrnlMigrator#test: jrnl should have date.  = 0.05 s
  1) Failure:
test: jrnl should have date. (TestJrnlMigrator) [/Users/parkermoore/code/jekyll-import/test/test_jrnl_importer.rb:23]:
<2013-09-24 02:06:00 -0400> expected but was
<2013-09-24 11:36:00 -0400>.

(As a side note, the first argument in assert_equals is the actual value, the second arg is the expected value.)

Why don't we make a decision to just change this such that it extracts the string and puts the string from the jrnl line and outputs it as-is?

aniketpant · 2013-09-26T16:57:48Z

@parkr What do you mean by the last part? Sorry, I didn't understand that.

parkr · 2013-09-26T17:07:18Z

Read in the (time) string, output the same string. Don't convert at all.

Signed-off-by: Aniket Pant <me@aniketpant.com>

parkr · 2013-09-29T01:41:54Z

Boom! LGTM. @mattr-?

mattr- · 2013-10-03T04:12:19Z

Cool. Thanks! 💚

New Feature: Import jrnl files

aniketpant added 2 commits August 9, 2013 22:56

Start writing new feature for jrnl import

08724a9

Signed-off-by: Aniket Pant <me@aniketpant.com>

Add new import feature for jrnl files

a685bba

Signed-off-by: Aniket Pant <me@aniketpant.com>

Add error if file not found

6db76e4

Signed-off-by: Aniket Pant <me@aniketpant.com>

parkr reviewed Aug 12, 2013
View reviewed changes

aniketpant added 3 commits August 13, 2013 10:03

Improve error message

96168d4

Signed-off-by: Aniket Pant <me@aniketpant.com>

Decouple code and add docs

9bb17cf

Signed-off-by: Aniket Pant <me@aniketpant.com>

Fix relative path issue

298f1e2

Signed-off-by: Aniket Pant <me@aniketpant.com>

aniketpant mentioned this pull request Sep 23, 2013

Support for Jekyll and Letterpress jrnl-org/jrnl#55

Closed

parkr reviewed Sep 23, 2013
View reviewed changes

aniketpant added 3 commits September 24, 2013 06:11

Revert date change and remove stray whitespace

8b796f3

Signed-off-by: Aniket Pant <me@aniketpant.com>

Improve error message

426d42b

Signed-off-by: Aniket Pant <me@aniketpant.com>

Add newline for YAML front data

b3c0421

Signed-off-by: Aniket Pant <me@aniketpant.com>

parkr reviewed Sep 24, 2013
View reviewed changes

aniketpant added 3 commits September 24, 2013 11:29

Fix silly typo

4093570

Signed-off-by: Aniket Pant <me@aniketpant.com>

Update methods in compliance to tests

4980e39

Signed-off-by: Aniket Pant <me@aniketpant.com>

Add tests

a067b94

Signed-off-by: Aniket Pant <me@aniketpant.com>

mattr- reviewed Sep 25, 2013
View reviewed changes

Fix unrequired string conversions

4d7b990

Signed-off-by: Aniket Pant <me@aniketpant.com>

parkr reviewed Sep 26, 2013
View reviewed changes

Fix date test

106de4d

Signed-off-by: Aniket Pant <me@aniketpant.com>

Change get_date code to fix issue with test

b579325

Signed-off-by: Aniket Pant <me@aniketpant.com>

mattr- added a commit that referenced this pull request Oct 3, 2013

Merge pull request #51 from aniketpant/import-jrnl

74005bd

New Feature: Import jrnl files

mattr- merged commit 74005bd into jekyll:master Oct 3, 2013

mattr- added a commit that referenced this pull request Oct 3, 2013

Update history to reflect merge of #51

94e9b57

aniketpant deleted the import-jrnl branch October 4, 2013 01:59

jekyll locked and limited conversation to collaborators Feb 27, 2017

jekyllbot added the frozen-due-to-age label Feb 27, 2017

New Feature: Import jrnl files #51

New Feature: Import jrnl files #51

Conversation

aniketpant commented Aug 12, 2013

Available overrides

Usage

aniketpant commented Aug 12, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Aug 12, 2013

aniketpant commented Aug 12, 2013

parkr commented Aug 12, 2013

aniketpant commented Aug 12, 2013

parkr commented Aug 12, 2013

aniketpant commented Aug 12, 2013

parkr commented Aug 12, 2013

aniketpant commented Sep 23, 2013

Choose a reason for hiding this comment

parkr commented Sep 23, 2013

aniketpant commented Sep 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Sep 24, 2013

aniketpant commented Sep 24, 2013

parkr commented Sep 24, 2013

parkr commented Sep 24, 2013

aniketpant commented Sep 24, 2013

parkr commented Sep 24, 2013

aniketpant commented Sep 25, 2013

Choose a reason for hiding this comment

mattr- commented Sep 25, 2013

parkr commented Sep 25, 2013

aniketpant commented Sep 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aniketpant commented Sep 26, 2013

parkr commented Sep 26, 2013

aniketpant commented Sep 26, 2013

parkr commented Sep 26, 2013

parkr commented Sep 29, 2013

mattr- commented Oct 3, 2013