Create New Files #5

Merged
merged 2 commits into from May 25, 2012

Conversation

Projects
None yet
2 participants

Building on top of the new file browsing code, these changes allow the users to create new files within the repo.

clowder commented May 24, 2012

Can you squash/cleanup the commits so that we don’t have WIP commits.

Also, whats that bugfix? Should it be its own pull request?

clowder commented May 24, 2012

Side note, you need to chase someone and sit with them to do a code review to get these merged before moving to the next ticket. Interdependent pull requests === bad.

@dazoakley dazoakley Make the changes needed to allow deagol to create files within the di…
…rectory structure.

And also it sets the format dropdown correctly on the create/edit page based on the file extension of the file.
c629abb

@clowder clowder commented on an outdated diff May 24, 2012

lib/gollum/frontend/app.rb
@@ -72,6 +72,12 @@ def set_the_path(file_path)
@name = params[:splat].first.split("/").last
wiki_options = settings.wiki_options.merge({ :page_file_dir => @path })
wiki = Gollum::Wiki.new(settings.gollum_path, wiki_options)
+
+ @format = nil
+ if match = /^.+\.(\w+)$/.match(@name)
+ @format = match[1]
+ end
@clowder

clowder May 24, 2012

This reads weird, having the assignment in the if condition. Something like...

@format = nil
matches = /.(\w+)$/.match(@name)
if matches 
  @format = matches[1]
end

or

@format = nil

if /.(\w+)$/ =~ @name
  @format = $1
end

Would be preferable.

@clowder clowder commented on an outdated diff May 24, 2012

lib/gollum/frontend/app.rb
@@ -105,13 +111,22 @@ def set_the_path(file_path)
end
post '/create' do
- name = params[:page]
- wiki = Gollum::Wiki.new(settings.gollum_path, settings.wiki_options)
- format = params[:format].intern
+ name = params[:page].split('/').last
+ path = set_the_path(params[:page].dup)
@clowder

clowder May 24, 2012

Would extract_path be a more descriptive name?

@clowder clowder commented on the diff May 24, 2012

lib/gollum/frontend/app.rb
@@ -105,13 +111,22 @@ def set_the_path(file_path)
end
post '/create' do
- name = params[:page]
- wiki = Gollum::Wiki.new(settings.gollum_path, settings.wiki_options)
- format = params[:format].intern
+ name = params[:page].split('/').last
@clowder

clowder May 24, 2012

Should this have an extract_name helper like below?

@clowder clowder commented on an outdated diff May 24, 2012

lib/gollum/frontend/app.rb
@@ -247,6 +262,10 @@ def show_page_or_file(name)
file.raw_data
else
@name = name
+ @format = nil
+ if match = /^.+\.(\w+)$/.match(@name)
+ @format = match[1]
+ end
@clowder

clowder May 24, 2012

Same comment as above.

Also, this repetition should be extracted into something like the extract_* helpers commented on above.

@clowder clowder and 1 other commented on an outdated diff May 24, 2012

lib/gollum/frontend/public/gollum/javascript/gollum.js
@@ -101,13 +101,15 @@ $(document).ready(function() {
$('#minibutton-new-page').removeClass('jaws');
$('#minibutton-new-page').click(function(e) {
e.preventDefault();
+ var path = $(this).attr('data-path') ? $(this).attr('data-path') : '';
@clowder

clowder May 24, 2012

Could be simplified to...

var path = $(this).attr('data-path') || ‘’;

Assuming we are using jQuery >=1.6.

@dazoakley

dazoakley May 24, 2012

No can do - 1.4.4 :(

@clowder

clowder May 24, 2012

Actually you should be able to just use the data api.

var path = $(this).data('path') || ‘’;

EDIT: This is moot if you use the below for setting the data path. It would never be empty.

@clowder clowder and 1 other commented on an outdated diff May 24, 2012

lib/gollum/frontend/views/create.rb
@@ -42,7 +42,7 @@ def page_name
end
def formats
- super(:markdown)
+ super(format().to_sym)
@dazoakley

dazoakley May 24, 2012

LOL! Been writing faaar too much javascript of late!

@clowder clowder and 1 other commented on an outdated diff May 24, 2012

lib/gollum/frontend/views/pages.rb
@@ -7,6 +7,12 @@ def title
"Browse Files"
end
+ def data_path
+ if @path && @path != '' && @path != '/'
+ " data-path=\"#{@path}/\""
+ end
@clowder

clowder May 24, 2012

Is the goal here just to ensure a trailing slash? Would something like this suffice.

if @path
  %Q{data-path="#{ File.join(‘./’, @path, ‘/') }/“} 
end

Would return...

> @path = ‘foo/bar’
=> ‘./foo/bar/‘

> @path = ‘/‘
=> ‘./‘

EDIT: relative paths!?

@dazoakley

dazoakley May 24, 2012

esentially yes, but we are also trying to exclude sending through ' data-path="//"' (coming when path == '/') or ' data-path="/"' (when path is '') as this will screw the file creation.

@clowder

clowder May 24, 2012

Above should achieve that, if you pass a / you’ll get ./ in response. Which should work right? My data should go to the relative root of the wiki?

@clowder

clowder May 24, 2012

Also, if you can always set a data-path attribute (even for /) you don’t need to check for undefined or ’’ in the javascript.

@dazoakley

dazoakley May 24, 2012

But if you pass '' you'll get .//.

But the path format the Wiki class is expecting is not the full path - i.e. ./bjc/style should be passed through as bjc/style/.

And if you're in the root directory, it expects nil to be passed.

@clowder

clowder May 24, 2012

Not quite. The magic of [File.join] is that you never get two slashes next to each other.

irb(main):001:0> File.join('./', '', '/')
=> "./"
@clowder

clowder May 24, 2012

Side note....

irb(main):013:0> '/foo/bar/' =~ /^\/?(.*?)\/?$/; $1
=> "foo/bar"
irb(main):014:0> '/foo' =~ /^\/?(.*?)\/?$/; $1
=> "foo"
irb(main):015:0> '/' =~ /^\/?(.*?)\/?$/; $1
=> “”

I KNOW REGULAR EXPRESSIONS

@dazoakley

dazoakley May 24, 2012

You guys didn't want me to break out the Perl stuff!!! :-/

Ok, i'll go with that one, but I think the original is more clear of the intent:

  • if the @path is not nil, an empty string or '/', put in a data-path attribute for the js to deal with.
  • else, don't put anything in - there is no path

Whereas now we're just doing string manipulation regardless...

@clowder

clowder May 24, 2012

The 3 condition logic is just looks messy and passing nil around is a bit of a code smell.

@dazoakley dazoakley added a commit that referenced this pull request May 25, 2012

@dazoakley dazoakley Merge pull request #5 from nature/create_new_file
Create New Files
27fbd79

@dazoakley dazoakley merged commit 27fbd79 into master May 25, 2012

clowder commented May 25, 2012

Would have been good if we could have waited to merge this. There is still a bit of duplication that need to be cleaned up (https://github.com/nature/deagol/pull/5/files#L1R80 https://github.com/nature/deagol/pull/5/files#L1R268)

We can still clean the code/duplication up - I know it's there and intend to do it, but in a seperate pull. :)

As there are are other branches dependent on this work, constant refactoring/rebasing on this means a lot of extra work in the other branches as you then have to then rebase onto each new commit and repeatedly resolve needless conflicts (Then repeat the process for every other subsequent change). Makes Darren a very grumpy old man... ;)

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