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

Parse more data #5

Merged
merged 18 commits into from
Jul 26, 2020
Merged

Parse more data #5

merged 18 commits into from
Jul 26, 2020

Conversation

mcronce
Copy link
Contributor

@mcronce mcronce commented Jul 8, 2020

This updates the lib to parse HP/MP, attributes, and each job's current/max XP. It'll tick a bunch of the boxes in #3.

…:get() into its own small function in (new) util.rs called load_url(); load_url() accepts an optional "subpage" argument so that we can, soon, load additional profile pages
…<ClassInfo> in the value field instead of Option<u32> for just the level (THIS COMMIT DOES NOT COMPILE)
@mcronce
Copy link
Contributor Author

mcronce commented Jul 8, 2020

Also just noticed I accidentally included Profile::all_class_info(); I can take this back out and put it in a separate PR, but do need it for my use case

@Roughsketch
Copy link
Owner

Thank you very much for taking an interest in this crate and making a PR for it!

In order to test this PR I've made a new test case. To get it into this PR I've made my own PR onto this branch so you can work with it. It uses a dead account to pull information from, so unfortunately it's not able to test out anything new but it can provide a base for seeing how well it scrapes data.

The only input I have is that for any new public function I would like there to be doc comments so that the auto-generated docs explain the usage of the method to new people. The ones I see missing are for the following:

  • Attribute struct definition
  • Attributes get
  • ClassInfo struct definition
  • Classes get (This one looks like my mistake)
  • Profile class_info and all_class_info

Besides that I would like for new information to be added to tests just to make sure that it grabs information correctly.

@mcronce
Copy link
Contributor Author

mcronce commented Jul 20, 2020

Just as a heads up, I haven't forgotten about this (nor the other stuff I want to add ;)), I just haven't had time to address these yet

@mcronce
Copy link
Contributor Author

mcronce commented Jul 26, 2020

Alrighty, I think this is good to go @Roughsketch - let me know if it looks like I've missed anything or you want anything changed.

Thanks!

@Roughsketch Roughsketch self-requested a review July 26, 2020 20:51
Copy link
Owner

@Roughsketch Roughsketch left a comment

Choose a reason for hiding this comment

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

Looks good to me. Going to merge this and then update the dependencies to their current versions before releasing 0.4.0.

Thanks for the PR!

@Roughsketch Roughsketch merged commit 94ac859 into Roughsketch:master Jul 26, 2020
@mcronce
Copy link
Contributor Author

mcronce commented Jul 26, 2020

No problem :) I have more coming once I have time that will check off more of the boxes in #3

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