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

resolve #33 #34

Merged
merged 2 commits into from
Mar 12, 2020
Merged

resolve #33 #34

merged 2 commits into from
Mar 12, 2020

Conversation

Guseyn
Copy link
Contributor

@Guseyn Guseyn commented Mar 12, 2020

No description provided.

@g4s8 g4s8 self-assigned this Mar 12, 2020
Copy link
Member

@g4s8 g4s8 left a comment

Choose a reason for hiding this comment

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

@Guseyn please check few comments

README.md Outdated
@@ -66,7 +66,7 @@ for more technical details.
### Naming policy

RPM may use different names to store metadata files in the package,
by default `NamingPolicy.DEFAULT` will be used, to change naming policy use
by default `NamingPolicy.DEFAULT` will be used, to change policy policy use
Copy link
Member

Choose a reason for hiding this comment

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

@Guseyn typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@g4s8 fixed.

* @param key Key as file name.
* @return Xml directives as SingleSource
*/
public SingleSource<Directives> xmlDirectives(final String key) {
Copy link
Member

Choose a reason for hiding this comment

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

@Guseyn I don't agree with the design of this method and class - it has too many responsibilities: reading file sizes, calculating hash sums, reading system timestamps and generating XML. I'd keep only XML logic here, e.g.:

class RepoXml implements Iterable<Directive> {
  private final Directives xml;

  public RepoXml openSize(long size) {
    return new RepoXml(this.dirs.add("open-size").set(size));
  }

  public RepoXml gzipHash(String hash) {
    return new RepoXml(this.dirs.add("checksum")./*..*/);
  }

  // and other methods to patch XML

  @Override
  public Iterator<Directive> iterator() {
    return this.dirs.iterator();
  }
}

Then you can call it from Repomd like this:

something.map(key -> new RepomXml().key(key))
  .zipWith(Single.fromCallable(() -> Files.size(this.temp)), (xml, size) -> xml.openSize(size))
  .zipWith(Single.fromCallable(() -> Files.size(this.gzip)), (xml, size) -> xml.gzipSize(size))
  .zipWith(new Checksum(this.gzip).sha(), (xml, hash) -> xml.openHash(hash))
  .zipWith(Single.just(System.timestamp()), (xml, time) -> xml.timestamp(time))
  .flatMapCompletable(xml -> new Update(temp).apply(xml))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@g4s8 suggestions are applied.

* @param rxsto RXTO wrapper
* @return Result from storage
*/
private static Completable resultFromStorage(
Copy link
Member

Choose a reason for hiding this comment

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

@Guseyn the name is too vague here, this method loads metadata file by location name in repomd.xml, maybe we can name it metaFile() which will return Single<Path> instead of using Path file parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@g4s8 fixed.

private static Completable resultFromStorage(
final List<XML> nodes,
final Path file,
final RxStorageWrapper rxsto) {
Copy link
Member

Choose a reason for hiding this comment

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

@Guseyn this class can be instantiated internally, since it's just a wrapper for object's state storage field. We can use non-static method and construct it right in the body. Method parameter looks redundant here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@g4s8 fixed.

@Guseyn
Copy link
Contributor Author

Guseyn commented Mar 12, 2020

@g4s8 plz see the updates.

@g4s8 g4s8 merged commit c06708d into artipie:master Mar 12, 2020
@0crat
Copy link

0crat commented Mar 12, 2020

This pull request #34 is assigned to @g4s8/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @g4s8/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

@0crat
Copy link

0crat commented Mar 12, 2020

Job #34 is already in scope

@0crat
Copy link

0crat commented Mar 12, 2020

Job audit: ARC didn't merge or close PR, performer @g4s8/z/z didn't make CR comments

@0crat
Copy link

0crat commented Mar 12, 2020

Job was finished in 2 hours, bonus for fast delivery is possible (see §36)

@0crat 0crat removed the scope label Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants