Skip to content

fixed redundant images folders#3295

Closed
DavNej wants to merge 2 commits into
mochajs:masterfrom
DavNej:issue/3117
Closed

fixed redundant images folders#3295
DavNej wants to merge 2 commits into
mochajs:masterfrom
DavNej:issue/3117

Conversation

@DavNej

@DavNej DavNej commented Mar 27, 2018

Copy link
Copy Markdown

This is my first contribution and here is what I did.

  • Looked everywhere in the code to see where each image was used.
  • Changed paths, organised the "mocha/images" folder with a subfolder "growl"
  • Removed unused images and folders.

WARN: I can't figure out how to run a test to ensure that what I did was correct. Please review and tell me if I forgot something or if I should proceed otherwise.

Thanks :)

@jsf-clabot

jsf-clabot commented Mar 27, 2018

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coveralls

coveralls commented Mar 27, 2018

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 90.0% when pulling 89a9845 on DavNej:issue/3117 into ffd760e on mochajs:master.

@Nokel81

Nokel81 commented Mar 28, 2018

Copy link
Copy Markdown

Two things, the first is why did you move them into a sub folder and the second. Are all those files really not needed? Some of the files may not be referenced explicitly but implicitly because they are hosted somewhere else. For instance the logo is used in the readme but has a version in the repo so that (I assume) it can be updated later if wanted

@outsideris

Copy link
Copy Markdown
Contributor

I think we should keep the images. It is assets which don't mean only to use it in code.

@boneskull

Copy link
Copy Markdown
Member

I feel like the original issue should have been more specific.

@boneskull boneskull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi! thanks for the PR. as others have commented, there are some problems here, so I'll enumerate them:

  • Please sign CLA
  • Restore all files under assets/
  • Move media/logo.svg into assets/

Relocating the Growl images into images/growl/ is good

@boneskull boneskull added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Apr 1, 2018
@DavNej

DavNej commented Apr 2, 2018

Copy link
Copy Markdown
Author

Hi @boneskull

I replaced all deleted images and placed them in the assets folder. I placed in it media/logo.svg and growl/. I also changed the paths in bin/_mocha and in lib/mocha.js so we still have only one media folder.

Furthermore, I already signed the CLA and when I revisit the page to sign, they tell me I already did.

@outsideris

Copy link
Copy Markdown
Contributor

@DavNej Thank you for your contribution.
We will update contributors in package.json with automatic script, see #3290.
Please remove your name in package.json. Don't worry. We will update your name in our package.json.

I'm not sure why license/cla said that you are not signed yet.

@boneskull

Copy link
Copy Markdown
Member

We will update contributors in package.json with automatic script, see #3290.

IMO it's not necessary for anyone to remove themselves from the package.json. The script will overwrite it anyway.

@boneskull

Copy link
Copy Markdown
Member

@kborchers can you please check on the CLA status for this?

@boneskull

Copy link
Copy Markdown
Member

@DavNej I wonder if you could amend your commit then force push? That might "jiggle the handle" on the CLA system, as it were.

$ git commit --amend --reuse-message=HEAD
$ git push -f

@kborchers

Copy link
Copy Markdown

The issue with the CLA signature is likely that the user in the commit doesn't match the user that signed. If you look at the commit, the user is not recognized by GitHub and doesn't match the user that opened the PR and likely signed the CLA. The commit will need to be updated with the correct user and email or the CLA needs to be signed with the user on the commit.

@DavNej

DavNej commented Apr 3, 2018

Copy link
Copy Markdown
Author

Hi, I had some login issues I fixed. In order to have some change to commit I removed my name from the contributors in package.json, merged, and then push (with 3 commits. Sorry). Now the signature of the CLA should be ok.
I learnt stuff :)

@outsideris

Copy link
Copy Markdown
Contributor

DavNej signed CLA, but David Nejar doesn't. I think you should update author for all commits.

@DavNej

DavNej commented Apr 3, 2018

Copy link
Copy Markdown
Author

Hi,
I tried the following commands in order to change the author for the first four commits but it doesnt seems to work.
I ran:
git rebase -i HEAD~6
then inserted
exec git commit --amend --author="DavNej <davnej.dev@gmail.com>" -C HEAD after each commit I wanted to change in the temporary file I was prompted. I saved and closed, then ran
git rebase --continue
Have I done something wrong?

@kborchers

Copy link
Copy Markdown

Unless the project would prefer separate commits, I would just squash everything into one commit with the correct author.

@boneskull

Copy link
Copy Markdown
Member

@DavNej I'm not sure. Just rebase interactively and squash all commits with the new author. I'd recommend doing this in your Mocha working copy:

$ git config user.name 'DavNej'
$ git config user.email 'davnej.dev@gmail.com'

...then squashing.

@boneskull boneskull added the type: chore generally involving deps, tooling, configuration, etc. label Apr 4, 2018
@DavNej

DavNej commented Apr 4, 2018

Copy link
Copy Markdown
Author

@boneskull
Yes I realised that was the problem of the first commits and I took care of that. now I would want to squah all the previous commits into one identified by DavNej (who signed the CLA).
is there a proper way to squash them all?

@outsideris

Copy link
Copy Markdown
Contributor

You can squash them all with git rebase.
And you can author for the commit like:

$  git commit --amend --author="DavNej <davnej.dev@gmail.com>"

@boneskull

Copy link
Copy Markdown
Member

@DavNej Here's a decent tutorial on squashing.

@boneskull

Copy link
Copy Markdown
Member

This will be ready to merge once we sort out the squash & CLA business.

DavNej added 2 commits April 17, 2018 17:01
@DavNej

DavNej commented Apr 17, 2018

Copy link
Copy Markdown
Author

After some struggle and some useful learning, I squashed the commits leaving only the 2 main commits. Thank you very much for your patience. Peace :)

@outsideris outsideris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@outsideris

Copy link
Copy Markdown
Contributor

I'm not sure when I can merge PR yet. So, I leave this because this is ready to merge. @boneskull

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

Labels

semver-patch implementation requires increase of "patch" version number; "bug fixes" type: chore generally involving deps, tooling, configuration, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants