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

path compare #8775

Closed
wants to merge 1 commit into from
Closed

Conversation

skv-headless
Copy link
Contributor

No description provided.

@TeatroIO
Copy link

I've prepared a stage to preview changes. Open stage or view logs.

@skv-headless
Copy link
Contributor Author

updated #5927 now it much simpler.

@jvanbaarsen
Copy link
Contributor

@skv-headless Can you attach an up to date screenshot of this feature? Thanks :)

@@ -7,12 +7,10 @@ def execute(current_user, source_project, source_branch, target_project, target_
# Note: Use satellite only when need to compare between two repos
# because satellites are slower than operations on bare repo
if target_project == source_project
Gitlab::CompareResult.new(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct behaviour that you don't use Gitlab::CompareResult anymore?
--Edit Nevermind, I see that class is removed al together below

@skv-headless
Copy link
Contributor Author

on 1024px
selection_004

@jvanbaarsen
Copy link
Contributor

@skv-headless What happends if no path is given? Is the compare still working without it?

@skv-headless
Copy link
Contributor Author

It will work like before.

@jvanbaarsen
Copy link
Contributor

@skv-headless Maybe you can add a placeholder in the text box saying: Optional - i.e.: example path

@@ -257,7 +257,7 @@
end

get '/compare/:from...:to' => 'compare#show', :as => 'compare',
:constraints => { from: /.+/, to: /.+/ }
:constraints => { from: /.+/, to: /.+/, path: /.+/ }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary spacing detected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skv-headless Can you please fix this Hound issue?

@skv-headless
Copy link
Contributor Author

selection_005
@jvanbaarsen is it ok?

@sspreitzer
Copy link

+1 I need this feature very much in the Swiss meteorological office

@jvanbaarsen
Copy link
Contributor

@randx What do you think about this overal feature? If you want this to be merged in I'll guide the rest of this PR.

@dzaporozhets
Copy link
Member

@jvanbaarsen I like idea of comparing certain directory or file. If it can be done in nice way (UI too) - I would like to have it

@jvanbaarsen
Copy link
Contributor

@skv-headless Are you still working on this?

@skv-headless
Copy link
Contributor Author

What should I do? If I understood correct @randx don't like UI, but I don't know how to improve it.

@sspreitzer
Copy link

@skv-headless: please remove the whitespace after /.+/ as mentioned from @houndci above.

@sspreitzer
Copy link

Just add a new commit to the branch used in the pull request and push the branch to GitHub. The pull request will automatically be updated with the additional commit.

@jvanbaarsen
Copy link
Contributor

@randx Would the way @skv-headless implemented it now (UI-wise) be acceptable as a first pass? We can improve upon it later on.

{style: 'font-size: 15px;'}
%br
%br

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this. We already have new mr button on this page

@dzaporozhets
Copy link
Member

We can improve upon it later on.

@jvanbaarsen my experience says it would be me 😄 . So I rather finish UI in this PR. Try to hide path text field and expand it only when needed. Same like we did for import URL on new project page

@jvanbaarsen
Copy link
Contributor

@randx Well, yeah.. it would probably be you :trollface: Ok. I like what you're proposing. @skv-headless What do you think?

@jvanbaarsen
Copy link
Contributor

This merge request has been closed because a request for more information has not been reacted to for more than 2 weeks. If you respond and conform to the merge request guidelines in our contributing guidelines we will reopen this merge request.

@jvanbaarsen jvanbaarsen closed this Jun 1, 2015
@sspreitzer
Copy link

:,(

@vladkolotvin
Copy link

@skv-headless Are you going to continue working on this task? This is very useful feature!

@dhoffend
Copy link

I'm missing this feature as well

@rutsky
Copy link

rutsky commented Feb 7, 2016

Ough, this is a killer feature for me, too bad it's abandoned!

I have huge PR (300+ changed files), and to get it reviewed I want to provide separate document with high level description of changes with links on some details in overall PR diff (i.e. links on diffs for specific files).
And nobody can show my changes in per-file mode! I tried GitHub, BitBucket, Phabricator's Differential, Reviewable, Review Board.

@erykciepiela
Copy link

We would also appreciate such feature. Is there any chance someone is working on this currently or we're waiting for a volunteer?

@charlespockert
Copy link

Definitely would like this - diffing 1 file gives you a history of the file rather than a history of the full commits and is so useful!

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

Successfully merging this pull request may close these issues.