-
Notifications
You must be signed in to change notification settings - Fork 6
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
Merge wh-questions library in its current state #502
Conversation
See #499 for these. If they used to pass in the trunk, that would be by mistake (empty gold or something like that). All these tests either are mostly empty or are my own tests which I know I wanted to delete. Emily and I agreed there is no reason to keep them around in the hopes that someone fixes them one day; too many SKIPPED tests is also bad and should be reserved for conscious decisions. If a test seems to be missing most of its content, there is probably little reason to keep it around. |
Yeah, this is by all means unfinished (and yes, a philosophical divide may emerge in a discussion like this, I suppose!). What I was after is not having to compute a value for a feature more than once. There are decisions which need to be made based on a combination of choices, for example: are there question particles which exhibit diverse behavior with respect to what type of questions they occur with? Having to compute that twice (or more than once) is not ideal. Now, what I strongly believe needs to be avoided is duplicate code (because typos and bugs); but what could be done instead of the "globals" is a function that gets called in all places. Then the value would be computed more than once but at least by the same place in the code. Let's open an issue about this. Yes, "enriched choices" sounds like something I am after here, perhaps. |
…ices.py which will be called whenever needed.
…ng back to what it was originally.
Alright, many thanks, @dantiston for the review. I addressed some things he pointed out and opened a couple issues for the remaining things which I do not want to address now. I will now finally merge this PR in and hopefully we will never see a PR of this size here ever again!! :) |
Thanks @dantiston for doing the review. I added some comments to issues you've raised. I agree that we should not introduce new globals and instead should work towards removing any that exist. |
Oh, sorry, I thought it was a new issue created as a response to this PR review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @olzama for making those changes! I think it looks good!
I think there are still some outstanding issues, such as renaming constants, changing the example type to a comment, some other pending clean-ups. But, it seems like there is consensus about what needs to change and how to change it, so I'll go ahead and accept! Please finish the clean-ups before merging.
It was actually; I created it thinking I won't address it just yet but then went ahead and did. Sorry for the confusion. |
Thanks a lot, @dantiston and @goodmami! |
Re style policies: given that we seem to fundamentally disagree, maybe we should have a zoom meeting some time, instead of discussing it further via the forum? I think the forum both (i) can come across not the way the person means, and (ii) actually brings out additional aggressiveness/defensiveness in people sometimes. So maybe a meeting would be preferable (for communication purposes rather than for making any specific decisions, if for no other reason than simply because it is not very clear how any decision could technically be made :) ). |
Sounds good. Let's set it up by email. |
OK....
I would like to merge the wh-library in now. This PR is huge, and I am not sure anybody will want to review it (which is fine).
Now that we started using Github, I suggested to Emily that the new libraries are no longer developed in the previous fashion, when the person would work on their library for months if not years. That results in overwhelmingly difficult merges and makes careful review virtually impossible.
Perhaps in the future, people work on short self-contained branches, one (sub)phenomenon at a time, whichever the minimal work item could be defined without breaking tests, and then comfortably merges into the trunk, not worrying about the website and the actual release of the library in full. The trunk can be separate from the website.
Now, this PR contains lots of various changes, most having to do with wh-questions but some having to do with things like ICONS updates and making verbs SPR-underspecified (and lots of other things SPEC-empty).
The regression tests have been updated mostly with respect to ICONS, as I fixed some of formerly broken appends and the ICONS started showing up. Conversely, I fixed a type in matrix.tdl which contributed ICONS which did not yet make sense, so, I got rid of those underspecified ICONS in some 50 tests.
There is (at least) one regression which I will not fix now (because I do not know how) but will file an issue: it is item 197 in the Russian wh test (wh-rus-dev). There is a new ICONS there which I don't want, but similar things have always been in other wh-tests. Not sure why this one is new. Must be a formerly broken append as well. Now, I don't want any of them, but they will stay for now.
There is still plenty of work to do. For example, I need to simplify the WH feature to only contain an OR (it currently also has an unused AND). I will file an issue.
The information structure in wh-questions currently is not right. Again, will file an issue and hopefully will address it some time soon.
But I want to merge this in and start working on short branches instead.