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

Identify base dewey call number and generate shelf keys #353

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Identify base dewey call number and generate shelf keys #353

merged 1 commit into from
Sep 14, 2021

Conversation

banukutlu
Copy link
Contributor

@banukutlu banukutlu commented Sep 2, 2021 •
edited
Loading

Building off of Stanford's traject repository, determines a base call number for Dewey classifications.

Currently experimenting with Lcsort gem to generat shelf keys for dewey numbers using the same method as LC numbers but needed to add a prefix to get them to be valid for lcsort. We can discuss about this approach, the other possibility is to implement our own dewey shelf key generator.

@banukutlu banukutlu added this to the 1.1.x milestone Sep 2, 2021
@banukutlu banukutlu requested a review from awead September 2, 2021 21:03
@banukutlu banukutlu linked an issue Sep 2, 2021 that may be closed by this pull request
Identify Base Dewey Call Numbers #318
Closed
2 tasks
awead
awead suggested changes Sep 7, 2021
View reviewed changes
Copy link
Contributor

@awead awead left a comment

Choose a reason for hiding this comment

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

Overall, this is fine. I have some comments but most of that is directed towards refactoring, and it may be too early to tell.

config/traject.rb Outdated Show resolved Hide resolved
lib/psulib_traject/call_number.rb Outdated Show resolved Hide resolved
lib/psulib_traject/call_number.rb Outdated Show resolved Hide resolved
lib/psulib_traject/holdings.rb Outdated Show resolved Hide resolved
lib/psulib_traject/processors/call_number/dewey.rb Outdated Show resolved Hide resolved
spec/lib/psulib_traject/processors/call_number/dewey_spec.rb Outdated Show resolved Hide resolved
@banukutlu banukutlu force-pushed the 318-dewey branch 3 times, most recently from bf04098 to 15de925 Compare September 9, 2021 08:56
awead
awead suggested changes Sep 9, 2021
View reviewed changes
Copy link
Contributor

@awead awead left a comment

Choose a reason for hiding this comment

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

I have one more change, and another suggestion. We don't have to do the LC/Dewey into Base merge, but I have a feeling that's where we're headed, but I will differ to you if you think it's too early to make that call.

lib/psulib_traject/call_number.rb Outdated Show resolved Hide resolved
lib/psulib_traject/processors/call_number/dewey.rb Outdated Show resolved Hide resolved
@banukutlu banukutlu self-assigned this Sep 9, 2021
@banukutlu banukutlu force-pushed the 318-dewey branch 2 times, most recently from 7eb7937 to 2bfc029 Compare September 10, 2021 18:58
@banukutlu banukutlu added on hold and removed on hold labels Sep 10, 2021
@banukutlu banukutlu force-pushed the 318-dewey branch 2 times, most recently from 6964d22 to 132b415 Compare September 10, 2021 20:38
@banukutlu
Copy link
Contributor Author

banukutlu commented Sep 10, 2021 •
edited
Loading

code climate never happy 😒 never easy going 😄

It is nagging for the call number class having 1 method more than allowed which is 20. sigh

@awead I created an issue off of it and I am OK to go with it as is unless you think otherwise.

I was gonna mark it as confirmed to get the CI to pass but for some reason it does not let me.

awead
awead suggested changes Sep 13, 2021
View reviewed changes
Copy link
Contributor

@awead awead left a comment

Choose a reason for hiding this comment

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

This looks great! I'm glad to see my suggestions actually worked. 😄 Just one suggestion, in response to your question about the classification parameter. Otherwise, I think this is ready to go.

lib/psulib_traject/holdings.rb Outdated Show resolved Hide resolved
@banukutlu
Copy link
Contributor Author

banukutlu commented Sep 13, 2021 •
edited
Loading

I have one more change, and another suggestion. We don't have to do the LC/Dewey into Base merge, but I have a feeling that's where we're headed, but I will differ to you if you think it's too early to make that call.

I just updated the PR so it merges to a preview branch to test it with sample records but after that check I am fine with this to be merged to main so we can do the further tests in QA with a full index. I feel comfortable since the browse feature is just soft released and the browse related Solr fields not affecting other existing fields.

Building off of Stanford's traject repository, determines a base call
dfaed7b 
number for Dewey classifications.

Using the Lcsort gem, creates shelf keys for dewey numbers to sort
in forward and reverse directions.
@banukutlu banukutlu changed the base branch from main to preview/318-dewey-browse September 13, 2021 17:21
@awead awead merged commit f96075c into preview/318-dewey-browse Sep 14, 2021
@awead awead deleted the 318-dewey branch September 14, 2021 13:07
@banukutlu banukutlu mentioned this pull request Oct 4, 2021
Merged
banukutlu pushed a commit that referenced this pull request Oct 4, 2021
Base call for number for Dewey classifications. (#353)
dd09213 
Using the Lcsort gem, creates shelf keys for dewey numbers to sort
in forward and reverse directions.
banukutlu pushed a commit that referenced this pull request Oct 4, 2021
Base call for number for Dewey classifications. (#353) (#362)
7e8263c 
Using the Lcsort gem, creates shelf keys for dewey numbers to sort
in forward and reverse directions.
@banukutlu banukutlu linked an issue May 18, 2022 that may be closed by this pull request
Generate and Index LC and Dewey Call Number Shelf Keys #289
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awead awead awead approved these changes

Assignees

@banukutlu banukutlu

Labels
None yet
Projects
None yet
Milestone
1.1.x
Development

Successfully merging this pull request may close these issues.

Identify Base Dewey Call Numbers Generate and Index LC and Dewey Call Number Shelf Keys
2 participants
@banukutlu @awead