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

Feature/update #14

Merged
merged 3 commits into from
Oct 14, 2019
Merged

Feature/update #14

merged 3 commits into from
Oct 14, 2019

Conversation

Regenhardt
Copy link
Contributor

Basically the first commit is only updating the library to netstandard21 and the examples to core3.

The second commit applies some code style recommended by VS, i.e. using the new using declaration. I can remove this if you like, this just kinda happened.

@Regenhardt
Copy link
Contributor Author

Sorry had an unnecessary change in the csproj, cleaned it up.

@ArcticEcho
Copy link
Member

ArcticEcho commented Oct 13, 2019

All the lines of code that now have discard assignments have been changed to use spaces instead of
tabs for indentation, which isn't really an issue in VS, but it kind of looks bad everywhere else.

I'll do a full review on monday. Thanks for the PR though! :)

@Regenhardt
Copy link
Contributor Author

Ok give me a second. Glad you saw that, as I usually prefer tabs too. Must be the new install having other standard settings.

@Regenhardt
Copy link
Contributor Author

So configuring my fresh install indeed helped.
It's possible I overshot a bit this time - I completely tabified the files I touched. How about keeping them tabified an also adding an editorconfig to the repo that automatically sets tabs as standard? Helps against this kind of thing😅.

@Regenhardt
Copy link
Contributor Author

I'd put just the essentials in the editorconfig for now - tabs, size 4, encoding utf8.

@Regenhardt
Copy link
Contributor Author

Since I'm updating anyway: Should I also update nuget dependencies and the assembly/nuget version? Would do that in another commit.

@ArcticEcho
Copy link
Member

Yeah, may as well update that too.

@Regenhardt
Copy link
Contributor Author

I updated the major version, since this is a breaking change to people not on netstandard21/core3. had to make some adjustments, since AngleSharp changed some namespaces and API names.

@Regenhardt
Copy link
Contributor Author

Man I would prefer having some automatic tests now, since I don't have an actually working bot yet 😅.

@ArcticEcho
Copy link
Member

ArcticEcho commented Oct 14, 2019

Yeah, automated testing was something I wanted to add as well. However, most functions require authentication to work; and failing to see a clean way of handling that, I just left it. :D

The sample code is really the only thing I can point you towards wrt your broken bot. As I mentioned before, feel free to open an issue if you need help.

Anyway, the PR looks fine. Thanks for your contribution!

@ArcticEcho ArcticEcho merged commit 7765c12 into SOBotics:master Oct 14, 2019
@Regenhardt Regenhardt deleted the feature/update branch October 14, 2019 19:20
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