Stephen,
Anyhow, changing ordering at this will actually introduce more work.
Sincerely yours,
Ivan Zlenko.
On Mon, 10 Feb 2025 at 17:47, Stephen O'Donnell
<sodonn...@cloudera.com.invalid> wrote:
> Ivan,
>
> Facts beat hand wavy assertions. You are of course wrong, there is a
> pattern in the code, but I was also wrong. The code has many more files
> with static imports first. I asked some AI to create me a script to check,
> and it gave me:
>
>
> import os
> import re
>
> def find_java_files_with_static_imports_first(directory):
> java_files_with_issues = []
>
> for root, _, files in os.walk(directory):
> for file in files:
> if file.endswith(".java"):
> file_path = os.path.join(root, file)
> with open(file_path, 'r') as f:
> content = f.read()
> static_imports = re.findall(r'^import static .+;$',
> content, re.MULTILINE)
> non_static_imports = re.findall(r'^import
> (?!static).+;$', content, re.MULTILINE)
>
> if static_imports and non_static_imports:
> first_static_import =
> content.find(static_imports[0])
> first_non_static_import =
> content.find(non_static_imports[0])
>
> if first_static_import < first_non_static_import:
> java_files_with_issues.append(file_path)
>
> return java_files_with_issues
>
> directory = '/Users/sodonnell/source/ozone2'
> files_with_issues = find_java_files_with_static_imports_first(directory)
>
> if files_with_issues:
> print("Java files with static imports before non-static imports:")
> for file in files_with_issues:
> print(file)
> else:
> print("No issues found.")
>
>
> Running that:
>
> % python3 find.py | wc -l
> 220
>
> Changing "if first_static_import < first_non_static_import:" to "if
> first_static_import > first_non_static_import:"
>
> % python3 find.py | wc -l
> 1552
>
> So it is much more common to have the static imports first. I checked a few
> of the output files by hand, and the output appears correct.
>
>
> On Mon, Feb 10, 2025 at 12:27 PM Ivan Zlenko <ivan.zle...@gmail.com>
> wrote:
>
> > Stephen,
> >
> > Look, it was my first line of thought to come up with a general pattern
> for
> > a checkstyle, so it will be easier to change and review.
> > But after going through the whole Ozone code base and creating 33
> branches
> > with fixes I can safely say - there is no pattern.
> >
> > Sincerely yours,
> > Ivan Zlenko.
> >
> > On Mon, 10 Feb 2025 at 16:41, Stephen O'Donnell
> > <sodonn...@cloudera.com.invalid> wrote:
> >
> > > >From my observation it is 50/50 for Ozone codebase in terms of
> > > static/non-static imports ordering.
> > >
> > > I happened to have 9 random files open in Intellij, and checked them.
> All
> > > had static imports last, and based on my experience on the project over
> > the
> > > last 5 years, statics are almost always last. Possibly that is an
> > Intellij
> > > default and that is why. So I would encourage actually checking this
> > 50:50
> > > assertion and picking the one that is actually correct, rather than
> > > creating unnecessary change in the code base. These kind of changes
> make
> > > backports to older branches more difficult, as they will nearly always
> > > cause conflicts.
> > >
> > >
> > >
> > > On Mon, Feb 10, 2025 at 10:20 AM Ivan Zlenko <ivan.zle...@gmail.com>
> > > wrote:
> > >
> > > > Sammi,
> > > >
> > > > From my observation it is 50/50 for Ozone codebase in terms of
> > > > static/non-static imports ordering.
> > > > As I've said before I've already finished with Ozone checkstyle for
> all
> > > > modules and now waiting on the first to be merged so I can create all
> > > > follow up pull requests.
> > > >
> > > > Sincerely yours,
> > > > Ivan Zlenko.
> > > >
> > > > On Mon, 10 Feb 2025 at 15:15, Sammi Chen <sammic...@apache.org>
> wrote:
> > > >
> > > > > Ivan,
> > > > >
> > > > > Thank you for the explanation. I adjusted the intellij
> configuration
> > > > > accordingly.
> > > > >
> > > > > There is one observation. I'm using "IntelliJ IDEA 2022.1.3" which
> > > seems
> > > > > by default to put static imports after normal imports, which is the
> > > > > opposite order of the new proposal.
> > > > >
> > > > > If we use the new order, then the majority of source code files
> need
> > to
> > > > be
> > > > > updated, which will be a lot of effort.
> > > > >
> > > > > Shall we keep using the current non-static first then static order?
> > As
> > > > long
> > > > > as we follow the same order, we can achieve a consistent style
> too.
> > > > >
> > > > > Bests,
> > > > > Sammi
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Mon, 10 Feb 2025 at 17:46, Ivan Zlenko <ivan.zle...@gmail.com>
> > > wrote:
> > > > >
> > > > > > Hi, Sammi.
> > > > > >
> > > > > > > BTW, I 'm not sure if Intellij has any way to help enforce
> these
> > > > rules
> > > > > > so it would be easy to follow them.
> > > > > >
> > > > > > It will be straightforward to set up these rules in IntelliJ
> Idea.
> > > > Right
> > > > > > now I have changes for all modules in Ozone to make them
> compliant
> > to
> > > > the
> > > > > > new checkstyle and all changes were done in an almost automatic
> way
> > > > using
> > > > > > the "Optimize imports on the fly" option.
> > > > > >
> > > > > > The only thing you need to do is the following:
> > > > > > 1. Set class count for * import into something like 999, to
> prevent
> > > any
> > > > > > attempt to add * imports.
> > > > > > 2. Set layout as following:
> > > > > > import static all other imports
> > > > > > <blank line>
> > > > > > import all other imports.
> > > > > >
> > > > > > > "customImportOrderRules" and "tokens", can we have a little
> > > > explanation
> > > > > > of the rule here?
> > > > > >
> > > > > > The rules are pretty simple: sort all imports in alphabetical
> > order,
> > > > > > separate in 2 groups starting with static imports and ending with
> > > > common
> > > > > > imports, groups separated by whitespace.
> > > > > >
> > > > > > Sincerely yours,
> > > > > > Ivan Zlenko.
> > > > > >
> > > > > >
> > > > > > On Mon, 10 Feb 2025 at 14:34, Sammi Chen <sammic...@apache.org>
> > > wrote:
> > > > > >
> > > > > > > Thanks Attila and Ivan working on this improvement.
> > > > > > >
> > > > > > > IIUC, the major enforced rules of imports are
> > > > > > >
> > > > > > > <module name="CustomImportOrder">
> > > > > > > > <property name="sortImportsInGroupAlphabetically"
> > > > > > > > value="true"/>
> > > > > > > > <property name="separateLineBetweenGroups"
> > > > value="true"/>
> > > > > > > > <property name="customImportOrderRules"
> > > > > > > > value="STATIC###THIRD_PARTY_PACKAGE"/>
> > > > > > > > <property name="tokens" value="IMPORT,
> > STATIC_IMPORT,
> > > > > > > > PACKAGE_DEF"/>
> > > > > > > > </module>
> > > > > > >
> > > > > > >
> > > > > > > "sortImportsInGroupAlphabetically" and
> > "separateLineBetweenGroups"
> > > > look
> > > > > > > straightforward.
> > > > > > > "customImportOrderRules" and "tokens", can we have a little
> > > > explanation
> > > > > > of
> > > > > > > the rule here?
> > > > > > >
> > > > > > > BTW, I 'm not sure if Intellij has any way to help enforce
> these
> > > > rules
> > > > > so
> > > > > > > it would be easy to follow them.
> > > > > > >
> > > > > > > Bests,
> > > > > > > Sammi
> > > > > > >
> > > > > > > On Mon, 10 Feb 2025 at 17:02, Attila Doroszlai <
> > > > adorosz...@apache.org>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi Ozone developers,
> > > > > > > >
> > > > > > > > Ivan Zlenko has started working on standardizing license
> header
> > > and
> > > > > > > > the order of imports.
> > > > > > > >
> > > > > > > > The first PR [1] adds the sample header [2], as well as
> > > checkstyle
> > > > > > > > rules for these two items. Rules will be initially disabled
> in
> > > all
> > > > > > > > submodules. The plan is to update submodules gradually, one
> > at a
> > > > > > > > time, fixing existing files and enabling the rules.
> > > > > > > >
> > > > > > > > For new files please start using the standard header [2] and
> > > import
> > > > > > > > order (shown in the PR description), even if the rule is
> still
> > > > > > > > disabled in the specific module. This will help avoid
> > accidental
> > > > > > > > violations if PRs are merged concurrently.
> > > > > > > >
> > > > > > > > We will help resolve conflicts in open PRs. Please avoid
> > merging
> > > > PRs
> > > > > > > > with old CI results.
> > > > > > > >
> > > > > > > > thanks,
> > > > > > > > Attila
> > > > > > > >
> > > > > > > > [1] https://github.com/apache/ozone/pull/7836
> > > > > > > > [2]
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/ozone/blob/966145d5e869263d53c516d36a0c02e058fd24b9/hadoop-hdds/dev-support/checkstyle/license.header
> > > > > > > >
> > > > > > > >
> > > > ---------------------------------------------------------------------
> > > > > > > > To unsubscribe, e-mail: dev-unsubscr...@ozone.apache.org
> > > > > > > > For additional commands, e-mail: dev-h...@ozone.apache.org
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
