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/118_footer_strip #141

Merged
merged 15 commits into from
May 5, 2021
Merged

feature/118_footer_strip #141

merged 15 commits into from
May 5, 2021

Conversation

astrochun
Copy link
Contributor

@astrochun astrochun commented Dec 16, 2020

Closes #118
See #203 (some changes here for interactive disabling for faster script run)

 - Add footer field to default INI config
 - ReadmeClass: define curation_dict
@astrochun astrochun added bug Something isn't working curation Pertains to aspects of curation, including workflow management p1 Issues affecting production, multiple users labels Dec 16, 2020
@astrochun astrochun added this to the v0.17.0 milestone Dec 16, 2020
@astrochun astrochun self-assigned this Dec 16, 2020
@astrochun
Copy link
Contributor Author

I believe this fixes the issue with the excess footer.

A few things:
I tested this with two stage deposit under review. One with the footer and one without the footer. It appears to work as intended. The implementation is done before html2text parsing, thus using the markup text directly from Figshare.

At a later point when we decide to update the metadata, we should make sure the formatting is identically to what is intended.

Copy link
Contributor

@damian-romero damian-romero left a comment

Choose a reason for hiding this comment

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

I couldn't find the two test deposits in stage that you referred to here.

However, I reviewed the changes and they all look fine. I think for the sake of completing this before winterbreak we can close this issue since we'll be implementing further deposit checks later on.

Copy link
Contributor Author

@astrochun astrochun left a comment

Choose a reason for hiding this comment

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

This feature won't be included in release/v0.17.0. The footer seem to vary with deposit.

For 12935162, the footer anchor appears to be: <br></p><br><br><hr><br><i>

ldcoolp/config/default.ini Outdated Show resolved Hide resolved
ldcoolp/config/default.ini Outdated Show resolved Hide resolved
@astrochun astrochun modified the milestones: v0.17.0, v0.18.0 Dec 17, 2020
@astrochun astrochun removed this from the v0.18.0 milestone Jan 25, 2021
@astrochun astrochun added p3 Tier-3 issues (lowest level) - will be addressed and removed p1 Issues affecting production, multiple users labels Jan 25, 2021
# Conflicts:
#	ldcoolp/curation/inspection/readme/__init__.py
@astrochun astrochun changed the base branch from develop to master May 4, 2021 16:41
@astrochun
Copy link
Contributor Author

astrochun commented May 5, 2021

I have successfully ran a README.txt construction using footer_check script through as many deposits as possible.
Nearly all README.txt footer stripping happened as expected.

Some notable issues with handling of footer in Summary:
Christopher Frost: 13963886 (two carriage returns instead of three afterwards)
Gilby Jepson: 14298227 (extra carriage return after no other data, possibly due to an extra \n that we don't strip)
Julie Miller: 12020763 (extra carriage return, six total. Most likely an issue with the metadata)

README_footer_check.zip

UPDATE: Adding a strip for \n fixes 13963886. This is good for now.

Copy link
Contributor Author

@astrochun astrochun left a comment

Choose a reason for hiding this comment

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

Changes look good

@astrochun astrochun merged commit a99404c into master May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working curation Pertains to aspects of curation, including workflow management p3 Tier-3 issues (lowest level) - will be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strip Figshare Description footer for README.txt
2 participants