Skip to content

Conversation

@dterol23
Copy link
Contributor

The proposal follows the pattern template and addresses #310 issue. This is the first time we contribute to the repository so we'll be more than happy to get feature and comments to improve the contribution. Thanks.

@spier
Copy link
Member

spier commented Apr 16, 2021

@dterol23 thank you for putting this together! At a first glance it looks awesome already 🎉

I will aim to read it through more carefully and leave comments in this PR.

Thank you already for putting in the work and sharing this with the InnerSource community!

@spier spier added the 📖 Type - Content Work Working on contents is the main focus of this issue / PR label Apr 16, 2021
@spier spier changed the title Initial patter to propose a new source code inventory pattern Proposal of a source code inventory pattern Apr 17, 2021
Copy link
Member

@spier spier left a comment

Choose a reason for hiding this comment

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

Great first version already @dterol23!

I left a couple of review comments and suggestions inline.

Besides those, some general comments:

  • Would it make sense to spell out any possible relationship between your pattern and other existing patterns? For example the InnerSource Portal?
  • Are there any screenshots or mocks that you can use from the inventory in use at Philips?

I hope you find the rest of the feedback useful as well.

And thanks again for sharing this topic with all of us!

@dterol23
Copy link
Contributor Author

@spier Thanks for the excellent feedback and guidance you provided. I think I cover it all but please, help to make a second round of review. By the way, I am having difficulties to act on the link check feedback. I think it is related to the new images I've added but I could not figure it out how to solve it. Any idea?

@spier
Copy link
Member

spier commented Apr 21, 2021

@dterol23 you are welcome :) Thanks for producing the next revision.

About the link check:
I think the link check fails because your files have extensions in all uppercase e.g. assets/img/source-code-inventory-mockup-dashboard.PNG, but you are referencing the image file in your markdown with a lowercase file extension.

Will take a look at the rest of your changes in the coming days.

@dterol23
Copy link
Contributor Author

@spier Good catch on the extension in uppercase. Fixed.

Copy link
Member

@spier spier left a comment

Choose a reason for hiding this comment

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

Great 2nd version!

I have left a couple of more questions for your review.

Normally we would wait with merging until all comment threads on the PR are resolved.

However if you are eager to get this merged, we can also publish this as is an initial pattern, and then do further reviews in subsequent PRs if you want.

Up to you, just let me know.

@dterol23
Copy link
Contributor Author

Thanks @spier I've made a couple of changes to deal with your 2nd round feedback.
Regarding closing the PR, I am fine either way. We use Kanban in our team and I always push to keep small work in progress and close/deliver outstanding stories as soon as possible. But I am also fine waiting for further review if that gets an opportunity to get more feedback soon.

@spier
Copy link
Member

spier commented Apr 22, 2021

@dterol23 I love iterations myself! 🎉

Let's make this the last iteration, and then get this PR merged, so that the pattern shows up in the repo for everybody to see. That may lead to further feedback in the future.

For the last round please see these inline comments here. I think I accidentally filed these outside of a review, which is likely why you didn't see them:
https://github.com/InnerSourceCommons/InnerSourcePatterns/pull/322/files

Feel free to accept/reject and comment on these as you see fit.

Then we get this PR merged in the main line.

@dterol23
Copy link
Contributor Author

Thanks @spier . I had not seen the other couple of suggestions indeed. I took them into account and make a couple of pushes. Please, take a look. I am good with current version.

@spier
Copy link
Member

spier commented Apr 23, 2021

Awesome! Let's get this live!

@spier spier merged commit 9386b80 into InnerSourceCommons:master Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📖 Type - Content Work Working on contents is the main focus of this issue / PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants