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

restructured API response #4

Merged
merged 4 commits into from
Dec 7, 2021
Merged

Conversation

Amamgbu
Copy link

@Amamgbu Amamgbu commented Nov 18, 2021

No description provided.

Copy link
Collaborator

@geohci geohci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two small comments that I think should be easy to address. Once they're complete, you can then focus about adapting to the new API structure! Thanks!

curr_text = curr_text[c["curr"]['offset']:c["curr"]['offset']+c["curr"]['size']].replace("\n", "\\n")
is_edit_type_found,wikitext,edit_type = is_edit_type(prev_text,c['prev']['type'])
#check if edit_type in edit types dictionary
if edit_types.get(edit_type,{}) and is_edit_type_found:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the is_edit_type_found variable is having an effect right now. My understanding is that if it's False, you probably don't want to do increment any counts (just continue). If I'm reading the code right, currently if it's False, it still leads to a clause that updates the edit_types dictionary. So maybe instead just add something like: if is_edit_type_found: continue after running the is_edit_type function. Then you don't have to include it in any of the other if/else statements. This applies to each of the insert/remove/change clauses (not just this specific one).

is_edit_type_found,wikitext,edit_type = is_edit_type(prev_text,c['prev']['type'])
#check if edit_type in edit types dictionary
if edit_types.get(edit_type,{}) and is_edit_type_found:
if edit_types.get(edit_type,{}).get('change'):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole if/else block can be simplified I think to the following: edit_types[edit_type]['change'] = edit_types[edit_type].get('change', 0) + 1
Again, same goes for the corresponding insert/remove clauses higher up in the code

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, Isaac! I'd implement these changes.

Amamgbu and others added 2 commits November 27, 2021 00:26
update tree differ to do moves and merge text changes into single blo…
@geohci
Copy link
Collaborator

geohci commented Dec 7, 2021

Some quick high-level thoughts:

  • Thanks for making the requested changes from the first PR set of comments
  • I'm okay with the .gitignore file being added if that's helpful (though in some ways, that might be better left as local because it's pretty environment-specific). This push includes changing the license and some .cfg and .pyc files though that I don't think belong. I assume the license was a mistake, but if there's a reason to change the license, let me know. Otherwise, please remove those from the PR.
  • I have some suggestions for the is_edit_type and is_change_in_edit_type but I think it might be best to merge in these changes as is (once the extraneous files are removed) and then address these two functions in a separate PR so that this one doesn't grow too large / block some of these smaller changes from being pushed through.

@Amamgbu
Copy link
Author

Amamgbu commented Dec 7, 2021

Some quick high-level thoughts:

  • Thanks for making the requested changes from the first PR set of comments
  • I'm okay with the .gitignore file being added if that's helpful (though in some ways, that might be better left as local because it's pretty environment-specific). This push includes changing the license and some .cfg and .pyc files though that I don't think belong. I assume the license was a mistake, but if there's a reason to change the license, let me know. Otherwise, please remove those from the PR.
  • I have some suggestions for the is_edit_type and is_change_in_edit_type but I think it might be best to merge in these changes as is (once the extraneous files are removed) and then address these two functions in a separate PR so that this one doesn't grow too large / block some of these smaller changes from being pushed through.

That was a mistake. I'd fix and make a commit.

@geohci
Copy link
Collaborator

geohci commented Dec 7, 2021

Thanks! Merging

@geohci geohci closed this Dec 7, 2021
@geohci geohci reopened this Dec 7, 2021
@geohci geohci merged commit 99066a1 into wikimedia:edit-types Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants