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.

Sign up for GitHub

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

Jump to bottom

Remove acs4 dependency #3120

Merged
merged 3 commits into from
Aug 26, 2020
Merged

Remove acs4 dependency #3120

merged 3 commits into from
Aug 26, 2020

Conversation

hornc
Copy link
Collaborator

@hornc hornc commented Mar 3, 2020 •
edited

Closes #3055
Closes #3119 assuming the correct answer is 'No, OL does not have a real dependency on acs4'

What does this PR achieve?

  • Allows local dev linting to pass by avoiding the external vendor acs4.py code from outside this repo
  • Removes a symlink (symlinks have caused problems on different OSs with Docker in the past)
  • Removes a seemingly unused dependency from /vendors

Technical

A call was made to a single method

https://github.com/internetarchive/acs4_py/blob/66780231ed8b32214d880b900ff275bb8379fbee/acs4.py#L561-L580

and the result was not used anywhere.

Testing

  • Needs to be tested somehow, apparently last time this was attempted there were problems?

Evidence

Stakeholders

@hornc hornc mentioned this pull request Mar 4, 2020
Merged
@hornc hornc changed the title WIP: Remove acs4 dependency Remove acs4 dependency Mar 5, 2020
@hornc hornc marked this pull request as ready for review March 5, 2020 09:38
@hornc hornc added the Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed] label Mar 5, 2020
cclauss
cclauss approved these changes Mar 10, 2020
View reviewed changes
Copy link
Collaborator

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

RSLGTM

@xayhewalo xayhewalo added the Needs: Investigation This issue/PR needs a root-cause analysis to determine a solution. [managed] label Mar 10, 2020
@mekarpeles mekarpeles assigned mekarpeles and cdrini and unassigned mekarpeles Apr 21, 2020
@hornc
Copy link
Collaborator Author

hornc commented Aug 11, 2020

@cclauss I just noticed this needed rebasing, and I have done so.

I have found acs4 causes problem running Python 3 locally in Docker. I haven't heard anyone else complaining about it.

AFAICT it is imported but not used, unless there is something the import is doing by default?

@cclauss
Copy link
Collaborator

cclauss commented Aug 11, 2020

I have found acs4 causes problem running Python 3 locally in Docker.

What problem?

@hornc hornc mentioned this pull request Aug 13, 2020
Open
@mekarpeles mekarpeles assigned mekarpeles and unassigned cdrini Aug 26, 2020
@mekarpeles mekarpeles merged commit ca91d1a into master Aug 26, 2020
@cdrini cdrini deleted the feature/3055 branch September 15, 2020 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss cclauss approved these changes

Assignees

@mekarpeles mekarpeles

Labels
Needs: Investigation This issue/PR needs a root-cause analysis to determine a solution. [managed] Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Does Open Library have a real dependency on acs4? Remove acs4 shortcut and import it directly from vendor
5 participants
@hornc @cclauss @mekarpeles @cdrini @xayhewalo