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

Merge from master #5302

Merged
merged 46 commits into from
Mar 19, 2019
Merged

Merge from master #5302

merged 46 commits into from
Mar 19, 2019

Conversation

caalador
Copy link
Contributor

@caalador caalador commented Mar 18, 2019

Merge master to 2.0


This change is Reviewable

Denis and others added 30 commits February 12, 2019 09:10
* Update byte buddy version

Fixes #4956
* Add m2e lifecycle mapping for maven-antrun-plugin
* Shortcuts no longer work, when element is disabled
* Document the fact that a component with a parent will be added to a new parent and removed from the previous one.
…sable node (#5142)

Fixes #4191

Defining focus-target is necessary for the grid cell delegating focus to the first focusable component within it when Enter key is used (tests available from another PR on vaadin-grid-flow repo)
* Add some configuration for surefire plugin

Due to issues with tests run under Windows 10 we need to
add the trimStackTrace and reuseForks configurations also
to the surefire plugin

* Also update the surefire and failsafe plugins.
Introduces ValueChangeMode#LAZY, and #TIMEOUT for delayed value synchronization to the server and makes Input implement HasValueChangeMode.
Check the function presence before calling it and take care about children property as well.

Fixes #5206
public void updateValue(Serializable newValue) {
if (isReadOnly()) {
LoggerFactory.getLogger(WebComponentBindingImpl.class)
.warn(String.format("An attempt was made to write to " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Invoke method(s) only conditionally. rule

public interface WebComponent<C extends Component> extends Serializable {
void fireEvent(String eventName);

void fireEvent(String eventName, JsonValue objectData);
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Document this public method by adding an explicit description. rule


void fireEvent(String eventName, JsonValue objectData);

void fireEvent(String eventName, JsonValue objectData,
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Document this public method by adding an explicit description. rule

void fireEvent(String eventName, JsonValue objectData,
EventOptions options);

<P extends Serializable> void setProperty(
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Document this public method by adding an explicit description. rule


import elemental.json.JsonValue;

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL First sentence of Javadoc is incomplete (period is missing) or not present. rule

<P extends Serializable> void setProperty(
PropertyConfiguration<C, P> propertyConfiguration, P value);

<P extends Serializable> P getProperty(PropertyConfiguration<C, P> propertyConfiguration);
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Document this public method by adding an explicit description. rule

* @param <C> {@code component} being exported
*/
public interface WebComponent<C extends Component> extends Serializable {
void fireEvent(String eventName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Document this public method by adding an explicit description. rule

* tag name of the web component
* @return {@link WebComponentConfigurationImpl} by the tag
*/
protected WebComponentConfigurationImpl<? extends Component> getConfigurationInternal(String tag) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove usage of generic wildcard type. rule

* @param <T> component
* @return set of {@link WebComponentConfiguration} or an empty set.
*/
public <T extends Component> Set<WebComponentConfiguration<T>> getConfigurationsByComponentType(Class<T> componentClass) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Document this public method by adding an explicit description. rule

* custom element tag
* @return Optional containing a web component matching given tag
*/
public Optional<WebComponentConfiguration<? extends Component>> getConfiguration(String tag) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove usage of generic wildcard type. rule

assert servletContext != null;

Object attribute;
synchronized (servletContext) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR "servletContext" is a method parameter, and should not be used for synchronization. rule

}
}

private static WebComponentConfigurationRegistry createRegistry(ServletContext context) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove this unused method parameter "context". rule

}
}

protected WebComponentConfigurationImpl<? extends Component> constructConfigurations(
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove usage of generic wildcard type. rule

*
* @return unmodifiable set of web component builders in registry
*/
public Set<WebComponentConfiguration<? extends Component>> getConfigurations() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove usage of generic wildcard type. rule

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 43 issues

  • CRITICAL 19 critical
  • MAJOR 20 major
  • MINOR 1 minor
  • INFO 3 info

Watch the comments in this conversation to review them.

10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MAJOR WebComponentWrapperTest.java#L229: Remove this use of "Thread.sleep()". rule
  2. MAJOR WebComponentWrapperTest.java#L290: Remove this use of "Thread.sleep()". rule
  3. MAJOR ClassesSerializableTest.java#L186: Define and throw a dedicated exception instead of using a generic one. rule
  4. MAJOR ClassesSerializableTest.java#L316: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed. rule
  5. MAJOR ClassesSerializableTest.java#L316: Define and throw a dedicated exception instead of using a generic one. rule
  6. MAJOR ClassesSerializableTest.java#L331: Reduce the total number of break and continue statements in this loop to use at most one. rule
  7. MAJOR ClassesSerializableTest.java#L359: This block of commented-out lines of code should be removed. rule
  8. MAJOR ClassesSerializableTest.java#L460: Use the built-in formatting to construct this argument. rule
  9. MINOR WebComponentWrapper.java#L43: Remove the "disconnect" field and declare it as a local variable in the relevant methods. rule
  10. INFO ClassesSerializableTest.java#L358: Complete the task associated to this TODO comment. rule

Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 58 of 58 files at r1.
Reviewable status: 33 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @caalador)

Copy link
Contributor Author

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Dismissed @vaadin-bot from 33 discussions.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all discussions resolved, 2 of 1 LGTMs obtained

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.