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

start moving unstable code to lens/private #270

Merged
merged 19 commits into from
Jan 11, 2016

Conversation

AlexKnauth
Copy link
Collaborator

see #269

@jackfirth
Copy link
Owner

I'm thinking some of these functions, like if-lens, cond-lens, and match-lens (and eventually lazy-lens and rec-lens) should go in the compound lenses section, while the stuff currently in there that's specific to a data structure should go into its data structure's section, like lens-join/vector. Putting lens-join/struct in with the other struct things makes sense to me, and the other lens joining functions should follow suit.

@AlexKnauth
Copy link
Collaborator Author

Ok, I'll do that as well then.

@jackfirth
Copy link
Owner

@AlexKnauth Is there more needed here? Also something I just realized - the unstable files reference the entire lens library with (require lens), but all the internal files reference through relative paths. I think things will break with cyclic dependencies when we do expose these files for v3.

@AlexKnauth
Copy link
Collaborator Author

Ok, I've just moved the join functions out of compound and into their data-structure directories.

@AlexKnauth
Copy link
Collaborator Author

I'm not sure what to do with unstable/lens/list.rkt (which contains reverse-lens and last-lens) or unstable/lens/view-set.rkt (which contains lens-set-all). I'm not sure what to call list.rkt in the lens/private/list directory, and I'm not sure where to put lens-set-all.

@AlexKnauth
Copy link
Collaborator Author

@jackfirth What do you think?

@jackfirth
Copy link
Owner

What you've done looks good to me. Will review this a bit more tonight and investigate the build failure.

@AlexKnauth
Copy link
Collaborator Author

The build is failing because of a cover failure (cover: no coverage information for "/home/travis/build/jackfirth/lens/unstable/lens/zoom.rkt"). Should the cover command be put back in after_success? We did that in a16132f to avoid problems like this. Was that undone for some reason?

@jackfirth
Copy link
Owner

I think I remember undoing that because we'd often get into a state where we'd have no coverage information for months because we wouldn't notice the command kept failing, which pretty much defeated the whole point. I've also set it up to only run cover on 6.3.

@AlexKnauth
Copy link
Collaborator Author

Should I take out that commit then?
My thinking was that a raco cover command failing, even though it isn't a good thing, shouldn't be bad enough that it should result in a travis failure.

@jackfirth
Copy link
Owner

Yeah, I think we should make it fail the build. You're right that it's not really that severe, but I just know I'll never notice it's broken otherwise.

@AlexKnauth
Copy link
Collaborator Author

Ok I just took out that commit. What should we do about the cover failure though?

@jackfirth
Copy link
Owner

I'd just see if telling cover to ignore the zoom.rkt file works and go with that if it does. Better to have coverage info for most of the repo than none of it, and just eyeballing the file it looks pretty well tested.

@AlexKnauth
Copy link
Collaborator Author

Ok, I added zoom.rkt to cover-omit-paths and that fixed it.

@jackfirth
Copy link
Owner

And the coverage check ran and passed, good

jackfirth added a commit that referenced this pull request Jan 11, 2016
start moving unstable code to lens/private
@jackfirth jackfirth merged commit b24d06d into jackfirth:master Jan 11, 2016
@AlexKnauth AlexKnauth deleted the move-unstable branch January 11, 2016 04:36
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.

2 participants