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

[RDY] Refactor path module #430

Closed
wants to merge 10 commits into from
Closed

Conversation

lslah
Copy link
Contributor

@lslah lslah commented Mar 30, 2014

No description provided.

@schmee
Copy link
Contributor

schmee commented Mar 30, 2014

Nice! I'm going to move some additional functions into path.c later today. ref #414

@lslah
Copy link
Contributor Author

lslah commented Apr 1, 2014

This is actually not much yet, but I'd prefer not blowing up PRs and thus would mark this as ready as soon as someone has reviewed the changes.

Also if anyone else wants to proceed on refactoring this module, feel free to do so. It'll probably take some days until I'd continue this.

@lslah lslah changed the title [WIP] Refactor path module [RFC] Refactor path module Apr 1, 2014
@schmee
Copy link
Contributor

schmee commented Apr 1, 2014

I'll take a look at this later today! I will look for more refactorings, after all it was my PR that created this mess ;)

@lslah
Copy link
Contributor Author

lslah commented Apr 1, 2014

@schmee Thanks, that'd be nice. And no, you haven't created the mess, but nicely extracted it in order to make it solvable. ;)

@lslah
Copy link
Contributor Author

lslah commented Apr 1, 2014

Unfortunately my last git --amend hides this discussion about constant names.

I think the general question is, whether constant names in enums should have a common prefix or not.

A common prefix makes it easier to classify a random constant in the code (i.e. you know which enum type the constant belongs to).
Not having to prefix a constant makes it sometimes easier to find readable, verbose names.

The constants which raised the question are the possible return values of path_full_compare formerly known as fullpathcmp:

kFilesEqual
kFilesEqualNames
kFilesDifferent
kFilesMissingBoth
kFilesMissingOne

vs.

kEqualFiles 
kEqualFileNames 
kDifferentFiles 
kBothFilesMissing 
kOneFileMissing

Maybe @tarruda can decide which way to go?

@schmee
Copy link
Contributor

schmee commented Apr 1, 2014

LGTM 👍

@lslah
Copy link
Contributor Author

lslah commented Apr 1, 2014

@schmee Thanks for reviewing! I'll just wait until the naming-issue above is solved before I mark this PR as ready.

@schmee
Copy link
Contributor

schmee commented Apr 1, 2014

As soon as this gets merged I'll start moving stuff from os/fs.c into path.c.

@lslah lslah changed the title [RFC] Refactor path module [RDY] Refactor path module Apr 2, 2014
@lslah
Copy link
Contributor Author

lslah commented Apr 2, 2014

Marked as ready, since I don't want to prevent @schmee from introducing further improvements in this module and the naming thing doesn't seem to be such a big issue.

@tarruda
Copy link
Member

tarruda commented Apr 3, 2014

Merged, thanks

@tarruda tarruda closed this Apr 3, 2014
@lslah lslah deleted the refactor-path-module branch April 3, 2014 16:37
@aktau
Copy link
Contributor

aktau commented Apr 4, 2014

@tarruda @lslah this still had the seperator vs separator spelling error in it. I also think we should still adhere to the style guide, now that we have one and are making changes.

@lslah
Copy link
Contributor Author

lslah commented Apr 4, 2014

@aktau Sorry for my mistake with separator in the test file. I fixed it in path.c and path.h but somehow overlooked the test-file. I'll fix this in a sepArate PR.
Can you please point out more specific what's wrong with the style in this PR?

@Hinidu
Copy link
Contributor

Hinidu commented Apr 4, 2014

@lslah In touched lines I see some /* */ comments, absence of curly braces in one-line ifs and not aligned arguments in function calls. Maybe @aktau talked about it.

@lslah
Copy link
Contributor Author

lslah commented Apr 4, 2014

@Hinidu You probably mean the definition of gettail_dir. I just moved this function to a sensible place near its usage but haven't actually modified the definition itself. Sure, I can also adapt this function to the style guide, but I think that would look as if it's already refactored as well. I also changed the name of four functions and touched therefore a lot of lines where there're called, but didn't adapt these lines to the style guide.

If there are also style issues in the functions I've refactored, I'd be glad if you could add line notes and I'd definitely fix them together with my misspelling.

@Hinidu
Copy link
Contributor

Hinidu commented Apr 4, 2014

@lslah I don't see style issues in refactored code.

dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 2017
`#! /usr/bin/env bash` is apparently a valid/used shebang (yes, with a
space even).
butwerenotthereyet pushed a commit to butwerenotthereyet/neovim that referenced this pull request Dec 30, 2019
Works only in s:execute of fzf's Vim plugin.
s:execute_term corrupts the output of fzf#shellescape.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants