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

Add content to format_metadata, different_tools and help #25

Merged
merged 8 commits into from
Nov 23, 2021

Conversation

hot007
Copy link
Contributor

@hot007 hot007 commented Nov 17, 2021

resolves #15 , resolves #16 , resolves #23

Addresses #24

Please review!
Need to check if anchor links work in different_tools page, too.

Thank you :)

@paolap
Copy link
Contributor

paolap commented Nov 17, 2021

I also made quite a lot of changes to different tools trying to test a different formatting and a lot more content but I haven't pushed them yet, so I might try to merge your branch locally with mine, it might take sometime though

@hot007
Copy link
Contributor Author

hot007 commented Nov 17, 2021

Ah, sorry Paola :( Please feel free to override my changes where needed - or if it's not too disparate then you could accept this then merge. I suspect I've just caused you a pile of pain though, sorry I should have done it as multiple pull requests in hindsight :(
Also see new comment on #24 which you may have already addressed in your changes.

@hot007
Copy link
Contributor Author

hot007 commented Nov 18, 2021

In hindsight we should definitely do one pull request per page!

Copy link
Contributor

@chloemackallah chloemackallah left a comment

Choose a reason for hiding this comment

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

Love the braindump in format_metadata

@hot007
Copy link
Contributor Author

hot007 commented Nov 18, 2021

I'm bad at maths - there's an error in format metadata where I estimate chunk size, it'll be out by a factor of 8 because I forgot that bits and bytes aren't the same thing :p so @chloemackallah that'll need to be fixed. I can do that on review but I'm not going to make any more edits until Paola's had a chance to deal with the mess I made her in the tools section.

@hot007
Copy link
Contributor Author

hot007 commented Nov 18, 2021

Then realised I can change that file without affecting @paolap so I've fixed that chunk size estimate. Thanks heaps for the review Chloe! :)

@hot007
Copy link
Contributor Author

hot007 commented Nov 19, 2021

DO NOT MERGE THIS PULL REQUEST!!!
It should not have gone to main branch. This PR is replaced by #27
Instead we should issue a PR to a branch on this repo and then merge branches.

hot007 added a commit that referenced this pull request Nov 23, 2021
Replacement PR to avoid merge problems. If this works then we can also accept #25, else I can delete that one and @paolap  can go from here.
@hot007
Copy link
Contributor Author

hot007 commented Nov 23, 2021

I think I can merge this one too now. Apologies for the mess!

@hot007 hot007 merged commit bc18f9e into ACDguide:main Nov 23, 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
3 participants