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

fixed redundant images folders #3295

Closed
wants to merge 2 commits into from
Closed

fixed redundant images folders #3295

wants to merge 2 commits into from

Conversation

DavNej
Copy link

@DavNej DavNej commented Mar 27, 2018

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
Copy link

jsf-clabot commented Mar 27, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Mar 27, 2018

Coverage Status

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

@Nokel81
Copy link

Nokel81 commented Mar 28, 2018

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
Contributor

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

@boneskull
Copy link
Contributor

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

Copy link
Contributor

@boneskull boneskull left a comment

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
Copy link
Author

DavNej commented Apr 2, 2018

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
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
Contributor

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
Contributor

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

@boneskull
Copy link
Contributor

@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

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
Copy link
Author

DavNej commented Apr 3, 2018

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
Contributor

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

@DavNej
Copy link
Author

DavNej commented Apr 3, 2018

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

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

@boneskull
Copy link
Contributor

@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
Copy link
Author

DavNej commented Apr 4, 2018

@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
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
Contributor

@DavNej Here's a decent tutorial on squashing.

@boneskull
Copy link
Contributor

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

DavNej added 2 commits April 17, 2018 17:01
@DavNej
Copy link
Author

DavNej commented Apr 17, 2018

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 :)

Copy link
Contributor

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

👍

@outsideris
Copy link
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