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

NTuple Omit Prefix Implementation #58

Merged
merged 8 commits into from
Jan 25, 2022
Merged

NTuple Omit Prefix Implementation #58

merged 8 commits into from
Jan 25, 2022

Conversation

ives1227
Copy link
Contributor

Copy link
Collaborator

@pwinckles pwinckles left a comment

Choose a reason for hiding this comment

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

Thanks for submitting the extension!

@ives1227
Copy link
Contributor Author

@pwinckles I've addressed all of the above comments and recommitted. Thanks so much for your timely review!

@ives1227
Copy link
Contributor Author

@pwinckles I'll get to these later tonight or tomorrow. Thanks!

Copy link
Collaborator

@pwinckles pwinckles left a comment

Choose a reason for hiding this comment

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

I pulled the PR and opened it in my IDE this time (the source of a few the nitpicks here) and noticed that you're using tabs whereas this project uses spaces. At some point I should get a linter setup, but, for now, please replace all tabs with four spaces.

Additionally, I tried to use the new extension and could not because the extension's spec file is missing. You need to add a copy of the extension spec to this directory https://github.com/UW-Madison-Library/ocfl-java/tree/main/ocfl-java-core/src/main/resources/ocfl-specs

ocfl-java-api/pom.xml Outdated Show resolved Hide resolved
ocfl-java-aws/pom.xml Outdated Show resolved Hide resolved
ocfl-java-core/pom.xml Outdated Show resolved Hide resolved
ocfl-java-itest/pom.xml Outdated Show resolved Hide resolved
ocfl-java-test/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@pwinckles
Copy link
Collaborator

I just committed some checkstyle rules that you can pickup if you rebase. I'm not a dogmatic code style person, but it is nice if fundamental things like spacing is consistent.

@ives1227
Copy link
Contributor Author

@pwinckles I made the requested changes again. Thanks for the checkstyles - that helped me clear up the discrepancies.

@pwinckles
Copy link
Collaborator

It appears that your PR is now including commits that were made to main. You need to rebase your PR so that it only includes your commits.

@ives1227
Copy link
Contributor Author

@pwinckles I was just typing this to you. I rebased on main because of the checkstyles and didn't realize it included all of these changes. I'll fix this when I have a chance.

@pwinckles
Copy link
Collaborator

@ives1227 I'm hoping to get a release out this week or next. Do you think you'll have time to rebase this PR?

@ives1227
Copy link
Contributor Author

@pwinckles I will try to get to this in the next day or two. Between the holidays and work demands, I didn't have a chance to look at this.

@ives1227
Copy link
Contributor Author

@pwinckles I believe this is all set now.

@pwinckles
Copy link
Collaborator

You seem to have reverted the PR back to its original state, undoing all of the revisions that were made as part of the review process

@pwinckles
Copy link
Collaborator

I created an example branch of what this should look like. Note, that the tabs are still not fixed in that branch.

I created that checking out your branch and doing the following:

# Rebase to remove the three most recent commits
git rebase -i HEAD~4

# Add the upstream if you don't have it
git remote add upstream [email protected]:UW-Madison-Library/ocfl-java.git

# Rebase from upstream
git rebase upstream/main

That should produce the same branch as mine. Then, when you push to origin to update this PR you'll need to use the --force flag.

Copy link
Collaborator

@pwinckles pwinckles left a comment

Choose a reason for hiding this comment

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

Getting there... There are still pom files that are modified in this PR that should not be

@ives1227
Copy link
Contributor Author

@pwinckles the files I had previously were 1.4.2-SNAPSHOT but when I rebased from your branch, they were 1.4.3-SNAPSHOT. So, I should be using 1.4.3-SNAPSHOT instead?

@pwinckles
Copy link
Collaborator

@pwinckles the files I had previously were 1.4.2-SNAPSHOT but when I rebased from your branch, they were 1.4.3-SNAPSHOT. So, I should be using 1.4.3-SNAPSHOT instead?

Yes, 1.4.3-SNAPSHOT is correct

@ives1227
Copy link
Contributor Author

ives1227 commented Jan 25, 2022

@pwinckles I put the correct snapshot version back in.

Copy link
Collaborator

@pwinckles pwinckles left a comment

Choose a reason for hiding this comment

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

Thank you for the submission!

@pwinckles pwinckles merged commit 5558bfd into OCFL:main Jan 25, 2022
@ives1227
Copy link
Contributor Author

@pwinckles thank you!

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