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

Introduction of Path question #44

Merged
merged 4 commits into from
Aug 21, 2018
Merged

Introduction of Path question #44

merged 4 commits into from
Aug 21, 2018

Conversation

AuHau
Copy link
Collaborator

@AuHau AuHau commented Aug 4, 2018

Adding new question for Paths and its verifications.
Its main features:

  • Expand home (eq. '~/some/path' to '/home/some_user/some/path) correctly according to OS
  • Enables to enforce existence and non-existence of the path
  • Enables verification of the path type (file vs. directory)
  • Does proper path validation across supported OSs

Please let me know if you want to merge this to your repo, if so I will polish it more (eq. tests coverage, docs etc.)

@AuHau
Copy link
Collaborator Author

AuHau commented Aug 4, 2018

Hmm I just noticed that there is some similar work in #41
Since there was not an update since March, I could integrate it in my PR, with @djf604 permissions...

@magmax
Copy link
Owner

magmax commented Aug 4, 2018

I love this feature. Please, do it and fix the travis problems; it is easy, just a couple of flakes ;)

@djf604
Copy link
Contributor

djf604 commented Aug 6, 2018

Hi! So I wrote some similar Path features a couple months back, but it failed some tests and I never really looked much into why or did anything after that (which was pure apathy and totally my bad). I think I add a couple more features than just the path stuff, but I'd have to go back and take a look at my commit messages to remember what all I implemented.

That being said I'm perfectly happy merging our work! Odds are there is some overlap but we can hopefully work that out.

@coveralls
Copy link

coveralls commented Aug 7, 2018

Coverage Status

Coverage decreased (-1.8%) to 87.91% when pulling 0c6d4b8 on AuHau:master into 85af721 on magmax:master.

@AuHau
Copy link
Collaborator Author

AuHau commented Aug 7, 2018

So from my side, this should be all :-) Let me know if you have some feedback about the code or anything else.

Regarding the @djf604 PR, I am really open on how to approach this. I would maybe suggest merging this PR as it should be pretty ready and then if @djf604 has time now, he can rebase and add his Path completion feature or if not, I could do it.

I took a quite similar approach when implementing the Path question, but I mostly focused on the validation part of the question, while @djf604 on the renderer...

AuHau added 2 commits August 6, 2018 19:38
Adding new question for Paths and its verifications.
Its main features:
 * Expand home (eq. '~/some/path' to '/home/some_user/some/path)
 correctly according to OS
 * Enables to enforce existence and non-existence of the path
 * Enables verification of the path type (file vs. directory)
 * Does proper path validation across supported OSs
@djf604
Copy link
Contributor

djf604 commented Aug 7, 2018

Sure, give me a couple days to take a look at it and re-familiarize myself. I'll figure this out by the end of the weekend, sound good?

@AuHau
Copy link
Collaborator Author

AuHau commented Aug 20, 2018

@magmax any update on this? :-)

@magmax magmax merged commit f753ee4 into magmax:master Aug 21, 2018
@magmax
Copy link
Owner

magmax commented Aug 21, 2018

sorry for the delay. I missed those messages.

Thank you for your work and poking me :)

@AuHau
Copy link
Collaborator Author

AuHau commented Aug 21, 2018

Awesome, thanks! :-)

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.

4 participants