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

#1388 Warning of newline characters, leading and trailing spaces on elements id #2644

Merged
merged 6 commits into from
Nov 11, 2022
Merged

#1388 Warning of newline characters, leading and trailing spaces on elements id #2644

merged 6 commits into from
Nov 11, 2022

Conversation

WaltonG
Copy link
Member

@WaltonG WaltonG commented Sep 25, 2022

Description

Check if elements id has newline characters, leading or trailing spaces and add a warning on the report

@mbastian mbastian self-requested a review September 26, 2022 19:10
mbastian
mbastian requested changes Sep 26, 2022
View reviewed changes
Copy link
Member

@mbastian mbastian left a comment

Choose a reason for hiding this comment

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

Hi @WaltonG and thanks for the PR. Some comments:

  • You missed adding the change to the Bundle.properties file. There needs to be an entry for the Special_Characters_In_Id key.
  • You only validate the edge list but I think it should be both nodes and edges. I suggest looking at the checkColorAlpha function right above. You could make your function generic by using the ElementDraftImpl class, which both NodeDraft and EdgeDraft inherit. The message should however be specific enough so the user knows if it's a node or an edge that is affected.
  • It would be great to have unit test for this feature.

Let me know if anything is unclear

@WaltonG
Copy link
Member Author

WaltonG commented Sep 26, 2022

@mbastian Thank you for the review. All is clear

@WaltonG WaltonG marked this pull request as draft October 3, 2022 05:14
@mbastian mbastian linked an issue Nov 9, 2022 that may be closed by this pull request
Add warning when element IDs match without special characters such as new lines #1388
Closed
@mbastian
Copy link
Member

mbastian commented Nov 9, 2022

Hey @WaltonG do you need any help on this one? Let me know!

@WaltonG
Copy link
Member Author

WaltonG commented Nov 10, 2022 via email

@WaltonG
Copy link
Member Author

WaltonG commented Nov 11, 2022

Hi @mbastian I have re looked the code but cant seem to get why the test fails

@WaltonG WaltonG marked this pull request as ready for review November 11, 2022 15:37
@WaltonG WaltonG requested a review from mbastian November 11, 2022 15:37
@mbastian
Copy link
Member

Hi @mbastian I have re looked the code but cant seem to get why the test fails

Thanks @WaltonG I had a look and made a commit to correct the test. I think the test failed because you were using an ad-hoc Report as opposed to the report coming from the container. We actually had a neat function in another class to test the existence of certain issues in a report. I extracted this function out and put it into a Utils so we could also use it in this test.

mbastian
mbastian approved these changes Nov 11, 2022
View reviewed changes
Copy link
Member

@mbastian mbastian left a comment

Choose a reason for hiding this comment

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

Good to go, thank you @WaltonG!

@mbastian mbastian merged commit 9fc3212 into master Nov 11, 2022
@WaltonG
Copy link
Member Author

WaltonG commented Nov 12, 2022

Thank you. I'll pick another good first issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbastian mbastian mbastian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add warning when element IDs match without special characters such as new lines
2 participants
@WaltonG @mbastian