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

Limit number of error/warning/info messages #845

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Conversation

abyrd
Copy link
Member

@abyrd abyrd commented Dec 21, 2022

Messages are added via a method only when set is below a certain size. If a set reaches the maximum size, a message is added explaining that some messages were omitted for brevity.

@abyrd
Copy link
Member Author

abyrd commented Dec 21, 2022

I'm testing this out and fixing a couple of bugs - needs a bit of work before final review.

Messages are added via a method only when set is below a certain size.
If a set reaches the maximum size, a message is added explaining that
some messages were omitted for brevity.
@abyrd
Copy link
Member Author

abyrd commented Dec 21, 2022

This is ready for final review. Tested as follows: create a remove-stops modification, remove several stops from the middle of a route or two that have lots of patterns, specifying a very large amount of dwell time to remove at each stop (e.g. 600 seconds). You have to remove the stops from the middle of the route, not the beginning or end of the route, in order to cause negative travel times. This will generate tons of warnings. In the UI I see only 5, with a message that some were omitted.

Copy link
Member

@trevorgerhardt trevorgerhardt left a comment

Choose a reason for hiding this comment

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

Looks good overall, but in the Github diff view it appears a lot of the changes were off by 1 space. Was the surrounding spacing incorrect or is the new spacing incorrect?

Related: I had set up google-java-format to run on save for all changed lines for a little while and I believe introducing that as part of a common configuration for development (along with "optimize imports" on save) will increase productivity by reducing bike-shedding in the long run. The trade-off is it will increase total lines of code changed in the short term.

@trevorgerhardt trevorgerhardt added optimization t0 Time level 0: think hours and removed optimization labels Dec 21, 2022
@abyrd
Copy link
Member Author

abyrd commented Dec 21, 2022

Good catch. That's a search and replace mistake. I really wanted to do some intelligent, syntax-aware search and replace but it wasn't working and I ended up doing it textually. I had searched for a string starting with a space (to find word beginnings), and mistakenly replaced it with one not starting with a space. The textual replace had also messed up some other unrelated method calls that happened to have similar names. I really just need to learn to properly use syntax-aware replacement in IntelliJ which would have avoided all these problems.

In principle I'd like to apply uniform formatting rules and regularly check that they're applied. In practice, most sets of formatting rules I've worked with in the past made certain code much messier or harder to read, or just failed to work properly. I'll have to check out google-java-format to see how it fares - I imagine it's improved since the last time I tried this kind of thing.

Ah, I see there's an IntelliJ plugin, I'll try that out. https://plugins.jetbrains.com/plugin/8527-google-java-format/
When you say you set the formatter to "run on save for all changed lines", is this what you mean (triggered within the IDE)?

I'd want to ensure all big whitespace changes are applied in separate commits from intentional code commits. Grouping whitespace changes together with code changes in the same commit, or even the same set of commits, can make it hard to review changesets. Maybe we could reformat a few key files in individual commits, and if we like the style we occasionally do project-wide or full-file reformats to get it out of the way.

I'll fix those space issues and update the PR.

@abyrd abyrd merged commit 9abdbbf into dev Dec 21, 2022
@abyrd abyrd deleted the limit-error-messages branch December 21, 2022 10:42
@trevorgerhardt
Copy link
Member

trevorgerhardt commented Dec 21, 2022

Yes, I was using that plugin.

To keep formatting changes separate I would definitely be in favor of running a formatter with the adopted format / code style repo wide, on all files, in a single PR. After that, we could adopt a common configuration that would keep files formatted properly for all new changes.

@abyrd
Copy link
Member Author

abyrd commented Dec 21, 2022

I tried it out on a couple of files. It looks good overall but the line width and two-space indent are a significant departure from what we've been using (and what is standard in a lot of other Java projects), and some of the punctuation and wrapping looks less clear. Obviously in the big picture consistency is more important than bikeshedding any one detail, but I'd like to try a couple of other formatters before settling on one. I've made a note to look into that.

@abyrd
Copy link
Member Author

abyrd commented Dec 21, 2022

Issue created at #846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t0 Time level 0: think hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants