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

Remove dir prefix from filename of tests under dir. #6386

Merged
merged 1 commit into from Mar 13, 2014

Conversation

4 participants
@cirosantilli
Contributor

cirosantilli commented Feb 23, 2014

Rename refactoring only commit. E.g.:

  • rename features/project/commits/commit_comments.feature to comments.feature
  • rename features/steps/project/project_wiki.rb to wiki.rb
  • rename features/steps/snippets/user_snippets.rb to user.rb

Advantages

  • its DRYer: before we repeated the directory name in the files it contain.
  • we type less: every time I want to select a project_X file I have to do pro<tab>X<tab>. This lets us do only X<tab> instead.
  • it is more uniform: certain tests were already breaking the dir prefix "rule" before this commit, e.g.:
    • features/steps/project/deploy_keys.rb already did not have the prefix before.
    • under steps, most files already don't contain the dir prefix. This removes it from the few that do.

Disadvantages

The only downside I can see is that if your editor only shows basenames on tabs, you can't tell some files apart. However:

  • when working on a branch, it is likely that you will only touch files under a single directory.
  • if you use a good editor (e.g. Vim) you can configure it to show parent/basename on tabs instead.

Therefore, I feel that the advantages outweight the disadvantages.

doubts

  • Exception was made for files such as project/project.rb, which are unchanged. What if we rename those as main.rb or shared.rb to be DRYer?
  • What if we rename all suffixes classes like PublicProjects < Spinach::FeatureSteps to prefix classes like ProjectsPublic, which is already the case for the majority of tests? This would make this refactoring even neater.
@zzet

This comment has been minimized.

Contributor

zzet commented Feb 23, 2014

👍

@jvanbaarsen

This comment has been minimized.

Contributor

jvanbaarsen commented Feb 23, 2014

@cirosantilli I like the changes you made, i have some doubts about your doubts section (😄)
I dont like the main.rb or shared.rb names, i think project/project.rb is perfectly ok.
I do like the part of the renaming of classes, although im not sure if its worth all the effort.

@dzaporozhets dzaporozhets merged commit 170340e into gitlabhq:master Mar 13, 2014

1 check passed

default The Travis CI build passed
Details
@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Mar 13, 2014

@cirosantilli I'm not big fan of refactoring but this naming looks better. Thank you!

@cirosantilli cirosantilli deleted the booktree:rm-dir-prefix branch Mar 13, 2014

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