Add json support, fix #10, ref XQF/xqf#12 #13
Conversation
|
@illwieckz Nice work! Could you please rewrite the first two commits using @exprojects current GitHub email? Thanks. |
|
I used the email address @exprojects used himself, see eXprojects/qstat-json/commit/1c56085.patch for example. Legally I think it's the better thing to do. |
|
@illwieckz GitHub does profile linking using email. Since first and foremost license legal requirement is to make proper attribution I believe it is better to use @exprojects' current email. |
|
I think the only user email Github reveals is the commit one. |
|
@illwieckz And that's the email those two commits should have too :-) Check out latest commits here |
|
Well, the better is to ask @exprojects himself if he wants I rewrite the commits with his |
|
I've emailed Steve to ask him which address he wants (he can also just add the former address to his GitHub profile to get profile linking on that commit). |
| xform_printf(OF, "\t\t</players>\n"); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Can we split these out to a separate file please e.g. json.c, as qstat.c is already massive.
|
so, the code now splits
|
|
Just one last thing about PR:
There is no PR in git, after a merge, a PR is nothing about a noisy additional commit in the worst case (like GitHub does), PR is just a social mechanism to say « hello, I have changes for you ». A pull request is just the name GitHub gives to the process to request a pull from a foreign branch, some other online hosting platform call it a merge request for example, and in the traditional form, it's just a request for pull, like there is request for comments. A pull request is nothing than requesting a pull from a foreign branch, git does not know what is a PR, and there is absolutely no rule in Git that said there must be only one commit per branch, because there is already two obvious rule: there is one commit per commit, and there is branches for multiples commits for changes that does not fit in one commit. Git was not created to have only one branch per commit, which is insane to maintain, and a request for pull is about branch… If a PR were about commits and not branch, it were not called a pull request but a cherry-pick request… Foreign commits are not pulled, they are cherry-picked, pull is for branches, branches is for multiple commit about the same effort, and jumbo commits that made too much things must be splitted, but branch with multiple commits designed for a unique purpose must not be splitted. |
|
Making progress, but the issues highlighted by the review need to be fixed in this PR and not in separate PR's to ensure the code base is in good state no matter which revision is used. Its great to use new PR's for other issues like general style cleanup and display split which effect other areas of the code but each new PR needs to pass review on its own prior to commit. Currently your PR's are all dependent on each other instead of being based off separate branches, which make them impossible to review :( By altering the structure of how they are committed it should be possible to all but eliminate these interdependencies. From what I can determine we should see the following PR's:
Keep up the good work :) |
I can't do that, it means redoing all the work from scratch, with risk to introduce new issues. The work was done in the order I use because it was the safest way to do this job. I can only do that:
|
| xform_printf(OF, "\t\t</players>\n"); | ||
| } | ||
|
|
||
| void xml_display_farcry_player_info(struct qserver *server) |
There was a problem hiding this comment.
github is fooled, it's already fixed
|
Ok, this was was painful but it's done, it needed to manually backports changes from #26 to #13, #23 and #24. No branch was able to rebase automatically, but I was able to use merge/cherry-pick to recreate each branch without editing files (except for the backports from #26). Merging the unused var PR #25 before the json pr #13 is undoable, it means backporting by hand all the changes to all other PR since merge will be broken, editing plenty of files by hand each time to just redoing the job in each history. It's the better way to introduce new issues and it is very unsafe and painful. The unused var issue is in the code since years, it's not a problem at all if it's fixed after the json support addition. Fixing unused var before or after introducing json support is the same to you (json does not depend on it): it was not fixed, it will be fixed, it's ok, it's just another PR unrelated that can be merged right after others PR unrelated. But fixing unused var before or after introducing json support is not the same to me, it means editing manually each branch for each other PR. It's unsafe for no gain. This is now the PR list: #13 Add json support PR #26 is now unneeded since all commits were backported to #13 or merged to #27. |
|
I've done some extra work with uncrustify to clean the source and I get very good results, so the PR #24 will just be pure noise in the history since I will submit a better one soon. So, I closed the PR #24 and rebuilded the PR #25 and #27 to be able to merge them without it. This is now the PR list: #13 Add json support |
|
What's the status on this? |
|
Hello @stevenh, I just reedited all my PR to include the iterator limit to match malloc in protocol listing, I reminds you the merge order:
|
|
Hi @stevenh , is this PR ok for you now? I hope it will be merged soon. |
|
Still looks like a number issues already raised, haven't been addressed in this PR. A secondary PR to fix them is not an option as this means we'll have bad code in the repo at the point this is merged, so I'm afraid I'm going to have to insist all issues are addressed before we can get this merged. |
|
Hi,
Please list them, "looks like" is not an argument. |
|
Hi, I fixed in this PR the temporary double hyphen |
|
Hi @stevenh , is there still something preventing the merge ? |
- add json support to games added since Steve Teuber's patches - use xform_names instead of name_xforms since Steven Hartland's refactoring - use xform_printf() instead of fprintf() for json output - remove commented out code - use %s for a const switch seems a bit wasteful, fixing it - do not use inline tests - explicitely test pointers with NULL - escape lone backslash character too - RFC 4627 says: “JSON text SHALL be encoded in Unicode. The default encoding is UTF-8.” - iterator limit now match malloc in protocol listing - use singl-hyphen -json-protocols and -json-version options
|
merged via #53 |
Hi @stevenh, this PR imports patches from the @exprojects's qstat-json fork.
This fork was based on a source tarball, so was amnesic to all the revision history, also, this fork included mistakenly some temporary build files.
I've rewritten by hand the two useful patches and I committed them with the original
author(Steve Teuber <steve<ad>exprojects.org>) and the originalauthor datefor each commit.After that I've modified the source to add json support to games that were added since the release tarball Steve used (like
starmade,dirtybomb,farmsimandksp) and fixed theventriloomission he did.After that I renamed
name_xfromstoxform_namesand used xform_printf() instead of fprintf() in json code, like in e777674 (Refactored output splitting out xform processing into its own files improving various portions of it.)So, some intermediate commits cannot build, but the whole PR builds. I think we can't do better than that without rewriting Steve's commits, which raises legal issues: it must be clear to know who have done what and when.
@skybon, you will probably be interested by this PR.