Requests with additional path elements should not match

[bwehrle]

Hi @VaughnVernon ,

This is a PR for the issue related to resource paths matching where they should not match. I think the code could be simplified, but I'm proposing a simple fix that just checks if there are more path elements remaining:
eg: /foo/{id} with request /foo/1234/bar should not cause a match

[ghost]

Hi @bwehrle ,

First of all, thanks for reporting and fixing the bug 😄!

If I may ask, why did you prefer to add the test case in ResourceBuilderTest instead of ActionTest?

On the other hand, a minor comment about indexOfNextSegmentStart function, if it is private better IMO.

Thanks for your time! We hope you like vlingo ^^

[VaughnVernon]

Thanks, @brian-wehrle-roche ! I agree with @aleixmorgadas comments. If you could please, make the indexOfNextSegment() private and move the test to ActionTest. Aleix is the vlingo-http lead/champion so I will leave approval and merging to him.

[bwehrle]

Hi, I probably wasn't familiar enough with the tests. I'll implement the suggested changes.

My feedback on the code in this particular areas is that could be made easier to understand. If you share that idea, I can take a swipe at that after completing this bugfix change...

[VaughnVernon]

My feedback on the code in this particular areas is that could be made easier to understand.

Thanks @brian-wehrle-roche !

I think what you mean is when to use which scope, public, protected, package, or private. Here are the ideas behind the decisions, which some are obvious but I will state them anyway.

• public is used for any method that must have scope to any client code that lives in different packages. This is true of Action::Action(...) because dependent applications must be able to access this constructor from any proprietary (customer/client) package.

• protected is used only for extending subclass of a given superclass. Currently in Action we don't allow extension, so there are no protected methods.

• package (default) is highly necessary for libraries, allowing access to behaviors and state that may be gained only by other library components located in the same package. Various top-level vlingo-platform components use this because they are ultimately libraries and require "secret" access to internal behaviors and data.

• private is used when no other component outside the declaring class may access a method or data, not even extending subclasses or package-related classes.

Thus, indexOfNextSegmentStart() is useful only inside an Action instance because it is for parsing a URL and the parsing process is already private. It would provide no benefit to a component outside the Action class to access this method, and it may even result in an exception. Thus, we want to hide this to from all other classes, even those in the same library package because it is no useful and possibly even harmful, (but probably not so much). Exposing as package scope could make authors of package-scoped client code think that they need to use it.

I hope this helps. It definitely should be placed in some contributor guidelines. We had a discussion on that months ago and I don't recall where this stands. Any recollection, @aleixmorgadas ? I think you were involved in that task.

[ghost]

@bwehrle, thanks for updating the PR! The code looks good to me 😄

The CI failure is due dependency issue, we're fixing it

[ghost]

I hope this helps. It definitely should be placed in some contributor guidelines. We had a discussion on that months ago and I don't recall where this stands. Any recollection, @aleixmorgadas ? I think you were involved in that task.

Yes, I created vlingo/vlingo-community repo to add the contributions guidelines.

@bwehrle , question unrelated to the PR, which difficulties did you find during the PR? Lack of documentation? Lack of contribution process? In which area do you think it we can help newcomers better? Any feedback is appreciated ^^

[bwehrle]

Hi @VaughnVernon , @aleixmorgadas,
Sorry, I should have qualified better my comment. Missing the "private" was just an oversight on my part; I don't disagree on how the accessors should be used, - the principles stated above seem standard or similar to others. A good item for the wiki, though!

Rather, my comment was that the matchWith() code in the path parsing seems like it could be written in a way that would make it easier to understand, as it was not necessarily easy to change. Your opinion?

Thanks @aleixmorgadas for the feedback. The build is now failing due to a lag in publishing actors 0.7.8, I assume.

[VaughnVernon]

Rather, my comment was that the matchWith() code in the path parsing seems like it could be written in a way that would make it easier to understand, as it was not necessarily easy to change. Your opinion?

@bwehrle What, you don't like my code? ;)

No doubt it could be simpler, although my recollection is that it was kind of tricky and at the time I was working in isolation with no one to complain. If you want to discuss with @aleixmorgadas and change it, that's fine with me. It's been a matter of higher priorities of essential features/functionality so not much time has been spent critiquing what exists. As always, simplicity is the aim, but in this order of priority: (1) simplicity of the user-facing API, and then (2) simplicity of the core implementation. This is not an excuse to write complex code, but I don't think that this situation is widespread. I leave it to you two to decide on the outcome of this based on current priorities.

[ghost]

The build is now failing due to a lag in publishing actors 0.7.8, I assume.

Yes, as soon as we fix it, we merge the PR :)

Rather, my comment was that the matchWith() code in the path parsing seems like it could be written in a way that would make it easier to understand, as it was not necessarily easy to change. Your opinion?

Yes, some parts of the code aren't as easy to understand as they should be. I understand in which situation they were made and I would say we can adopt a methodology of improvement when a bug/new feature is related into that part of the code base.

I wouldn't like to be too much "code perfectionist" everywhere, I share the @VaughnVernon mindset about prioritizing the user-API instead of internal implementation when you cannot have both.

We are in a stage where vlingo-http is usable. Thus, we can put more effort into adding features meanwhile we do a more understandable code.

We could debate if it's worth to refactor this part of the code, but I think you will find more exciting develop new features ^^

Are you open to develop some new feature? 😄

[bwehrle]

Good morning @aleixmorgadas and @VaughnVernon, I don't disagree with anything you've said. However, when running across something that's improveable I take it as an opportunity to refactor. I find both aspects satisfying, new features and cleaner code. Also, refactoring is a great way to learn a new code base.
Regards,
Brian

[bwehrle]

Hi all,
I see the same problem persists, and the 0.7.8 vlingo-http is still not published to maven central.
Just a question: would it not be useful to apply the change I did for vlingo-actors to use jcentral (see the .travis.settings.xml file)?
Regards,
Brian

[ghost]

Hi @bwehrle. Yes, we had the discussion about using jcenter. We thought it was easier to fix the sync between jcenter and maven central. But the problem still persists.

After your PR, we did a couple of things to improve how we help newcomers, we opened a Gitter chat rooms and made the vlingo-community repository with some information. Do you think that information could helped you better? Or it was easy to collaborate without that information? Any feedback is appriciated 😄

• https://gitter.im/vlingo-platform-java/community
• https://gitter.im/vlingo-platform-java/http
• https://gitter.im/vlingo-platform-java/actors

I will speed up the merge ^^

[VaughnVernon]

Just a question: would it not be useful to apply the change I did for vlingo-actors to use jcentral (see the .travis.settings.xml file)?

Hi Brian, I actually had to rollback your change because we could not longer push from Travis to Bintray. I have a continuing Issue in with Sonatype to try to get us back from square one where we have a pipeline working. Since trying to support snapshots our Travis > Bintray > Sonatype > Central pipeline has been broken (one month). You can read the whole sob story here, but my latest comment summarizes the experience and why we are stuck:

https://issues.sonatype.org/browse/OSSRH-45028

[VaughnVernon]

@brian-wehrle-roche Our team member @d-led was able to identify the root cause of all the Bintray > Sonatype issues. You can read about it here in my most recent comment:

https://issues.sonatype.org/browse/OSSRH-45028

[brian-wehrle-roche]

It seems like this is a case where a better error message would have helped tremendously. What an ordeal! So once this is fixed the SonaType repo can be used for the artifact repo, since it will contain snapshots & released builds. Regards, Brian

…

On Mon, Jan 14, 2019 at 6:14 AM Vaughn Vernon ***@***.***> wrote: @brian-wehrle-roche <https://github.com/brian-wehrle-roche> Our team member @d-led <https://github.com/d-led> was able to identify the root cause of all the Bintray > Sonatype issues. You can read about it here in my most recent comment: https://issues.sonatype.org/browse/OSSRH-45028 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AqbzToA6GyK4cmCm9TFiVcGsfJlUilMcks5vDBJLgaJpZM4ZyLFr> .