-
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
Added more examples and test to Meronym and Holonym #28
Conversation
Hi, I would rather have a single pull request with the documentation close to complete, rather than adding things bit by bit. Or make it clear what is done and what not done. Currently, this is still missing:
Linking examples to OMW can maybe be left until the macro issue is resolved |
If I get you correctly, documentation for all relations should be complete before I raise a PR? And what should I do about this particular PR after adding those extras you pointed out? |
You can either redo this PR, or withdraw it and make a new one.
…On Wed, Sep 16, 2020 at 2:15 PM Glory Agatevure ***@***.***> wrote:
If I get you correctly, documentation for all relations should be complete
before I raise a PR? And what should I do about this particular PR after
adding those extras you pointed out?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#28 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIPZRQMQP5MYWZPWJRLFJLSGBJYZANCNFSM4RNCUHJQ>
.
--
Francis Bond <http://www3.ntu.edu.sg/home/fcbond/>
Division of Linguistics and Multilingual Studies
Nanyang Technological University
|
Let me interject. This is not the main branch but the gsod one. Personally I think small merges are fine, maybe even preferable. I'd rather not have to review a PR that does too many things at once. @gconnect Before withdrawing the PR, let me talk this over with Francis and get back to you so we agree on the proper procedure. |
Apparently you don't have to withdraw, just add commits and it will update the pull request. |
@gconnect Hi, I spoke with Francis and I think we agree. Let me try to summarize. Small pull requests are fine, but it's best if they can be "complete" in some sense. This means that if the pull request is for adding information about 1 or 2 relations, all the relevant information is included. Furthermore, the pull request should not bundle unrelated changes (e.g., a UI or settings change). The purpose of these guidelines is to make reviewing the pull request easier (it's easiest when we can clearly see all the relevant changes without other noise) and to keep the Git history clean (so later we can point to where something changed). Francis didn't realize that you can keep adding commits to the same pull request, which is why he suggested closing it and opening a new one. Since that is possible, there's no need to close this one. I hope that clarifies things! |
I am not sure what to do with the information provided here https://metacpan.org/pod/WordNet::QueryData |
* Rebuild the sidenav bar Rebuild the sidenav bar so that we can use build.py to generate it. * Remove local jquery Use <script src="https://code.jquery.com/jquery-3.4.1.slim.min.js" integrity="sha384-J6qa4849blE2+poT4WnyKhv5vZF5SrPo0iEjwBvKU7imGFAV0wwj1yYfoRSJoZ+n" crossorigin="anonymous"></script> instead * fix the plus icon bug the icon bug has been fixed and the css has been revised per Francis's suggestions.
* Rebuild the sidenav bar Rebuild the sidenav bar so that we can use build.py to generate it. * Remove local jquery Use <script src="https://code.jquery.com/jquery-3.4.1.slim.min.js" integrity="sha384-J6qa4849blE2+poT4WnyKhv5vZF5SrPo0iEjwBvKU7imGFAV0wwj1yYfoRSJoZ+n" crossorigin="anonymous"></script> instead * fix the plus icon bug the icon bug has been fixed and the css has been revised per Francis's suggestions. Co-authored-by: yoyo-go <[email protected]>
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.
It is hard to review this because the merged commits are showing up as diffs for this PR as well. It would be better if the new commits were rebased on the the others so only the new ones are included here.
gwadoc/doc_en.py
Outdated
""" | ||
relations.meronym.ex.en = "hand/finger" | ||
relations.meronym.exe.en = """ | ||
* *<sense: pwn-3.0:04574999-n:wheel>* ``meronym`` *<sense: pwn-3.0:02958343-n:automobile>* | ||
* *<sense: pwn-3.0:03138534-n:crown>* ``meronym`` *<sense: pwn-3.0:03497657-n:hat>* | ||
* *<sense: pwn-3.0:05217168-n:human body>* ``meronym`` *<sense: pwn-3.0:00007846-n:person>* |
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.
For these I think we will do our own substitution, so they can be changed to a form like this:
* `wheel <ILIURL/61096>`_ is a ``meronym`` of `wheeled vehicle <ILIURL/61100>`_
Where ILIURL
is that exact string and it will get replaced during the build process.
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.
I will effect the changes. Thanks
How do I go about it? |
Actually that's a good question. I don't want to make things too difficult, so maybe we should just proceed with the duplicated commits for now, and I will cherry-pick the relevant commits when I merge the gsod branch into the master branch. In other words, don't worry about it for now. |
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.
Just a few minor things. Once those are fixed I'll handle the merge. Please only commit the changes to doc_en.py
and do not commit the generated HTML files.
I resolved the conflicts locally then merged. After the merge I noticed that the examples for meronym and holonym are backwards and/or need to be double-checked. I'll take a look at that tomorrow. |
I pushed fixes for the examples in 1619432 |
What this PR Does
This PR fixes #26 and #31
Todo:
edit: changed "fixes" issue from 28 to 26